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

feat: add --expect-results and --exepect-result-count to npm query #5966

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Dec 14, 2022

This will allow users to tell npm whether or not to exit with an exit
code depending on if the command had any resulting entries or not.

@wraithgar wraithgar requested a review from a team as a code owner December 14, 2022 19:19
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Dec 14, 2022

found 1 benchmarks with statistically significant performance improvements

  • app-medium: no-cache

found 1 benchmarks with statistically significant performance regressions

  • app-medium: cache-only
timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 48.922 ±9.37 19.985 ±0.02 18.397 ±0.17 23.266 ±3.11 3.570 ±0.23 3.453 ±0.09 2.870 ±0.10 12.881 ±0.16 2.678 ±0.09 4.762 ±1.01
#5966 45.712 ±0.75 19.826 ±0.10 18.267 ±0.05 21.372 ±0.83 3.363 ±0.01 3.398 ±0.06 2.623 ±0.02 12.794 ±0.00 2.637 ±0.01 3.979 ±0.15
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 30.397 ±1.58 15.453 ±0.12 12.839 ±0.03 15.202 ±0.16 3.121 ±0.02 3.230 ±0.04 3.151 ±0.18 10.143 ±0.10 2.642 ±0.07 3.603 ±0.04
#5966 29.553 ±1.50 15.301 ±0.09 14.277 ±0.18 15.234 ±0.14 3.067 ±0.06 3.125 ±0.07 2.746 ±0.09 9.899 ±0.07 2.512 ±0.03 3.620 ±0.09

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lukekarrys
Copy link
Contributor

Rebased onto latest to fix conflicts

@lukekarrys
Copy link
Contributor

i pushed a commit converting to the new test config format which uncovered an interesting bug. the config type definition is converting all numbers to booleans (https://github.com/npm/cli/actions/runs/3820159448/jobs/6498253762#step:7:70).

with nopt there doesn’t seem to be a way to make a config that can be both a number and boolean that i could find?

@wraithgar
Copy link
Member Author

Well fiddlesticks, that's frustrating. We'll have to either get creative or ditch the numbers for now.

@wraithgar
Copy link
Member Author

So far from what I've seen discussed, the big asks have been for boolean, and for "more than one entry" (think react hoisting).

@wraithgar wraithgar self-assigned this Nov 13, 2023
@wraithgar wraithgar changed the title feat: add --expect-entries to npm query feat: add --expect-results and --exepect-result-count to npm query Feb 6, 2024
@wraithgar wraithgar force-pushed the gar/query-exit-code branch 3 times, most recently from 871ddc9 to afd9cc8 Compare February 6, 2024 17:52
This will allow users to tell npm whether or not to exit with an exit
code depending on if the command had any resulting entries or not.
Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Approved with one question that is not a blocker.

lib/base-command.js Show resolved Hide resolved
@wraithgar wraithgar merged commit 95b5057 into latest Feb 22, 2024
41 checks passed
@wraithgar wraithgar deleted the gar/query-exit-code branch February 22, 2024 16:57
@github-actions github-actions bot mentioned this pull request Feb 22, 2024
@cklim24

This comment was marked as spam.

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.

5 participants