-
Notifications
You must be signed in to change notification settings - Fork 3.9k
chore(refactor): promisify completion scripts #2759
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
Conversation
8b31c04 to
aaec756
Compare
|
Looks like something in the last patch to npm brought in more fs.promises references. That'll have to be fixed before CI is green. |
aaec756 to
4fd1322
Compare
nlf
left a comment
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.
this is an excellent refactor to remove some weirdness
that said, we should pick one way to define the completion function and stick with it. we have currently
function (opts) {}
async function (opts) {}
opts => {}
(opts) => {}
async (opts) => {}
i think the last one makes the most sense to use as the canonical definition style since we use arrow functions for most things, and just always declaring it async is nice for consistency
|
@nlf very good point. I tried to be aware of that when I was changing the tap functions (I think I got them to all be |
4fd1322 to
182b362
Compare
nlf
left a comment
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.
🎉
182b362 to
5ae2dc1
Compare
We also removed the "none" script because we handle a missing script just fine. There is no need to put an empty one in PR-URL: #2759 Credit: @wraithgar Close: #2759 Reviewed-by: @nlf
5ae2dc1 to
2ed0cc8
Compare
We also removed the "none" script because we handle a missing
script just fine. There is no need to put an empty one in