-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adapt to ES6 modules #538
Adapt to ES6 modules #538
Conversation
Pull Request Test Coverage Report for Build 3253589896
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't got the available time or sanity to go through this line by line, but I've scanned through it and pointed out the things that stood out. I didn't really take much notice of changes outside lib
and test
as others have already had a look and TBH they probably know more about those things than I do.
As long as the tests pass and you pick up the missing bit of stringify.property
I found then I approve. It would be useful if you put this into Thunderbird (even hacking it in temporarily) and ran the tests there, including the address book tests that handle vCards.
I was much hoping someone from the team could test this in Thunderbird, I don't have the setup currently so it would take me a bit to get back into the groove. |
Hey @darktrojan @leftmostcat @metasansana Here is my ES6 adaption of the library. I'd love to get this merged before we do any other major work, rebasing just the few changesets was a pain an prone to failure. I can help adapt pending PRs afterwards.
With the amount of changes you'll have to see how much review is useful, be sure to ignore whitespaces while reviewing, it will save you about 5k lines and makes reading easier!