-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Remove unused arguments on various function calls #2766
Conversation
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.
one line removed shouldn't have been, but the rest of this looks right to me
@@ -16,8 +16,7 @@ const usageUtil = require('./utils/usage.js') | |||
const usage = usageUtil( | |||
'npm profile enable-2fa [auth-only|auth-and-writes]\n', | |||
'npm profile disable-2fa\n', | |||
'npm profile get [<key>]\n', | |||
'npm profile set <key> <value>' |
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.
not sure why this was removed, we definitely have npm profile set
implemented
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.
I looked at other calls to usage.js. This call is missing trailing +
-- it's not supposed to be a variadic function. I put set
back and joined all of the strings with +
.
I also noticed that access.js incorrectly had its first argument as 'npm access'
instead of 'access'
.
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.
good catch, we definitely had the param to usage wrong here!
Locally, 10.1 fails on |
@@ -16,8 +16,7 @@ const usageUtil = require('./utils/usage.js') | |||
const usage = usageUtil( | |||
'npm profile enable-2fa [auth-only|auth-and-writes]\n', | |||
'npm profile disable-2fa\n', | |||
'npm profile get [<key>]\n', | |||
'npm profile set <key> <value>' |
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.
good catch, we definitely had the param to usage wrong here!
you're correct. that's an unrelated breakage that we're working on fixing. |
This is probably gonna collide heavily with #2772 so I will make sure these changes are represented there. |
I checked cli's code with Typescript using the tsconfig below. The compiler found a few arguments that are not used, so I removed them. In the case of `npm whoami`, it is clearer that it ignores its `args` and instead relies on `npm.flatOptions`. ```json { "compilerOptions": { "moduleResolution": "node", "module": "commonjs", "resolveJsonModule": true, "target": "es2019", "noImplicitAny": false, "noImplicitThis": true, "strict": true, "maxNodeModuleJsDepth": 0, "noEmit": true, "allowJs": true, "checkJs": true, "types": ["node"], "lib": ["esnext"] }, "include": ["lib"] } ``` PR-URL: npm#2766 Credit: @sandersn Close: npm#2766 Reviewed-by: @nlf, @ruyadorno, @Matausi29
448b893
to
b33c760
Compare
I checked cli's code with Typescript using the tsconfig below. The compiler found a few arguments that are not used, so I removed them.
npm whoami
, it is clearer that it ignores itsargs
and instead relies onnpm.flatOptions
.npm profile
, it turns out that it was missing+
between its string arguments; the test baselines show that this fixes profile's help output.