Skip to content

chore(refactor): promisify completion scripts#2759

Closed
wraithgar wants to merge 4 commits intolatestfrom
gar/completion-refactor
Closed

chore(refactor): promisify completion scripts#2759
wraithgar wants to merge 4 commits intolatestfrom
gar/completion-refactor

Conversation

@wraithgar
Copy link
Member

@wraithgar wraithgar commented Feb 23, 2021

We also removed the "none" script because we handle a missing
script just fine. There is no need to put an empty one in

@wraithgar wraithgar requested a review from a team as a code owner February 23, 2021 00:00
@wraithgar wraithgar force-pushed the gar/completion-refactor branch from 8b31c04 to aaec756 Compare February 23, 2021 00:06
@wraithgar
Copy link
Member Author

wraithgar commented Feb 23, 2021

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.

@wraithgar wraithgar force-pushed the gar/completion-refactor branch from aaec756 to 4fd1322 Compare February 23, 2021 04:37
@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes labels Feb 23, 2021
nlf
nlf suggested changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@nlf nlf 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 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

@wraithgar
Copy link
Member Author

wraithgar commented Feb 24, 2021

@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 async t =>, but obviously forgot to do that to the actual function I was refactoring. I'll change that.

@wraithgar wraithgar force-pushed the gar/completion-refactor branch from 4fd1322 to 182b362 Compare February 24, 2021 17:15
nlf
nlf approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

🎉

@wraithgar wraithgar force-pushed the gar/completion-refactor branch from 182b362 to 5ae2dc1 Compare February 24, 2021 18:05
@wraithgar wraithgar added the release: next These items should be addressed in the next release label Feb 24, 2021
@wraithgar wraithgar mentioned this pull request Feb 24, 2021
Merged
wraithgar and others added 4 commits February 25, 2021 09:44
@wraithgar
chore(package-lock): resetdeps and [email protected]
b9fa7e3
@wraithgar
[email protected]
28d036a
@kumavis @wraithgar
fix(explain): mark when dependency is bundled
983d218 
When using `npm explain <package>` it's useful to see if the package has
been bundled. This is especially useful when trying to understand the
provenance of a package's content

PR-URL: #2750
Credit: @kumavis
Close: #2750
Reviewed-by: @nlf
@wraithgar
chore(refactor): promisify completion scripts
2ed0cc8 
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
@wraithgar wraithgar force-pushed the gar/completion-refactor branch from 5ae2dc1 to 2ed0cc8 Compare February 25, 2021 17:52
@wraithgar wraithgar closed this in 113b131 Feb 25, 2021
@wraithgar wraithgar deleted the gar/completion-refactor branch November 2, 2021 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nlf nlf nlf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wraithgar @nlf @darcyclarke @kumavis