Skip to content
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

Promisify tests and packager function #658

Merged
merged 18 commits into from
Jun 5, 2017
Merged

Promisify tests and packager function #658

merged 18 commits into from
Jun 5, 2017

Conversation

malept
Copy link
Member

@malept malept commented May 29, 2017

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Converts all tests to use Promises (and gets rid of the waterfall dependency), and adds Promise support to the packager function via pify.

This is technically a breaking change, because the return value of packager has changed.

Fixes #627.

@malept malept requested a review from MarshallOfSound May 29, 2017 18:54
@malept malept force-pushed the promisify branch 2 times, most recently from 46c9240 to 863010a Compare June 4, 2017 20:03
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record these are massive changes and I've reviewed to the best of my understanding of the changes, when GitHub doesn't show you the diff of a JS file because it is too big you know you've got a large PR 😆

The logic conversion seems sound and I don't think any nitpicks I might have over code style should block this PR being merged especially when linting seems OK with everything 👍

@malept malept merged commit 3c8848c into master Jun 5, 2017
@malept malept deleted the promisify branch June 5, 2017 05:42
@Jmclerck
Copy link

I assume from looking at the release notes for the most recent versions of electron-packager this change hasn't been released yet? If not the Docs are a little confusing as it suggests that the Promisified versions of the API should already work when in fact they don't.

Is there an ETA on a release containing these changes?

Thanks!

@malept
Copy link
Member Author

malept commented Jun 19, 2017

All of the docs are based on the current state of master. If you want the docs for the released version, you'll need to look at the docs for a given version's tag.

I have about 3 more breaking changes to do before I release, and I do all of this work in my spare time, so I don't have an ETA, sorry.

@Jmclerck
Copy link

Hi @malept thanks for the clarification and quick reply. No problem about the ETA, I just wanted to check in case you had any idea.

Thanks for all your work on this, it is appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants