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 internals #112

Merged
merged 4 commits into from
Sep 26, 2020
Merged

Promisify internals #112

merged 4 commits into from
Sep 26, 2020

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented Sep 25, 2020

Started as a lint but, given the overlap, I think it's best to promisify first. No need to lint code that won't exist.

Best reviewed without whitespace.

This isn't a complete promisification, it just includes @octokit/plugin-throttling.js and changes a few callbacks to promises to make it more readable, catch more errors and reduce code along the way.

Excluding the dependency change, this PR reduces our code by 129 lines 148 lines 🥳

@fregante fregante force-pushed the lint-further branch 4 times, most recently from c04e366 to 5db6f2e Compare September 25, 2020 21:21
@fregante fregante changed the title Modernize further Promisify Sep 25, 2020
@fregante fregante changed the title Promisify Promisify internals Sep 25, 2020
@fregante fregante marked this pull request as ready for review September 25, 2020 22:02
@fregante
Copy link
Collaborator Author

I'll drop the async dependency as well

@fregante fregante marked this pull request as draft September 25, 2020 22:08
@fregante fregante marked this pull request as ready for review September 25, 2020 22:28
src/github-factory.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
Copy link
Owner

@denis-sokolov denis-sokolov left a comment

Choose a reason for hiding this comment

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

This is great, thank you! A few minor comments to resolve, but I love the PR.

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/should-delete-fork.js Show resolved Hide resolved
src/should-delete-fork.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@denis-sokolov
Copy link
Owner

Thanks for all the effort! And the commits in this PR are clear and atomic, thank you.

In my workflow once a PR is approved and no comments are outstanding, it’s up to the author to merge at their convenience. There may be reasons for delay: perhaps some more last-minute ideas pop up, or a merge needs an oversight for deployment or otherwise. So feel free to merge when convenient. Or let me know if you prefer a different workflow!

@fregante fregante merged commit 3ebc96d into master Sep 26, 2020
@fregante fregante deleted the lint-further branch September 26, 2020 22:30
@fregante
Copy link
Collaborator Author

fregante commented Sep 26, 2020

I’m glad to be of help! And I’m glad that Octokit had an elegant way to throttle the API that allowed this PR to actually make it faster.

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

Successfully merging this pull request may close these issues.

2 participants