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

Remove unused arguments on various function calls #2766

Merged
merged 1 commit into from Mar 4, 2021

Conversation

sandersn
Copy link
Contributor

@sandersn sandersn commented Feb 23, 2021

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.
  • For npm profile, it turns out that it was missing + between its string arguments; the test baselines show that this fixes profile's help output.
{
    "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"]
}

@sandersn sandersn requested a review from a team as a code owner February 23, 2021 23:56
@darcyclarke darcyclarke added Needs Review Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes labels Feb 24, 2021
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.

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>'
Copy link
Contributor

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

Copy link
Contributor Author

@sandersn sandersn Feb 24, 2021

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'.

Copy link
Contributor

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!

@sandersn
Copy link
Contributor Author

Locally, 10.1 fails on latest and on this branch, so I'm pretty sure those test failures are not a result of this change.

@@ -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>'
Copy link
Contributor

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!

@nlf
Copy link
Contributor

nlf commented Feb 24, 2021

Locally, 10.1 fails on latest and on this branch, so I'm pretty sure those test failures are not a result of this change.

you're correct. that's an unrelated breakage that we're working on fixing.

@ruyadorno ruyadorno added release: next These items should be addressed in the next release and removed Needs Review labels Feb 26, 2021
@wraithgar
Copy link
Member

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
This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants