Skip to content
This repository was archived by the owner on Jul 28, 2021. It is now read-only.

Conversation

@noelleleigh
Copy link
Contributor

This PR adds tink deprecate as a command, based on the functionality of npm deprecate.

Code ported from: https://github.com/npm/cli/blob/9a564a55c016642bff3e4ae58ed05a0d6370aaf3/lib/deprecate.js

Changes:

Tested conditions:

  • tink deprecate <pkg>[@<version>] <message>
  • tink deprecate <pkg>[@<version>]
  • tink deprecate <pkg> <message>
  • tink deprecate <pkg>[@<malformedversion>] <message>

Since I don't have an npm account, I just get to the last step of the process before receiving an HTTP 401 Unauthorized response. Is there a test registry that is used for API testing, or should I just work on the main one?

@zkat
Copy link
Contributor

zkat commented Nov 9, 2018

How do you feel about pulling in otplease?

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

Looks fine if it works as-is! I'm surprised by yargs magic sometimes


return BB.try(() => {
// fetch the data and make sure it exists.
const p = libnpm.parseArg(argv['pkg@version'])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so strange. Is this seriously handled just like that by yargs?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I first thought I would have to manually join the package name and the version, but then I ran the debugger and spotted that:
annotation 2018-11-09 170846

Fortunately it seems to work the way we want, but even the yargs docs don't seem to document what happens when two positional arguments are character-separated instead of whitespace-separated. I can't find a more comprehensive document for how it parses command templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

mind=blown

Choose a reason for hiding this comment

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

From what I can tell from the code it seems to just consider those cases as one positional argument, which makes sense.

@noelleleigh
Copy link
Contributor Author

How do you feel about pulling in otplease?

I could definitely give it a shot tomorrow.

@zkat
Copy link
Contributor

zkat commented Nov 9, 2018

One more thing: I just pushed test/ping.js as an example of writing network tests for tink. Would you mind adding test cases for this and getting the command some decent coverage %? (if not 100%)? It should be fairly straightforward if you base it off the ping test.

@noelleleigh
Copy link
Contributor Author

re: otplease

cli/lib/utils/otplease.js depends on cli/lib/utils/read-user-info.js for its otp() method.
Should I:

  1. Bring over the entirety of read-user-info.js (add read and npm-user-validate to dependencies), or

  2. Just bring over otp() (only have to add read to dependencies)?

@zkat
Copy link
Contributor

zkat commented Nov 11, 2018

Just pull in what you need. We don't need all of read-user-info.js tbh. Just a simple prompter that can be just as easily hand-rolled. Or written with ink.

@noelleleigh
Copy link
Contributor Author

One more thing: I just pushed test/ping.js as an example of writing network tests for tink. Would you mind adding test cases for this and getting the command some decent coverage %? (if not 100%)? It should be fairly straightforward if you base it off the ping test.

I added tests for deprecate based on the current npm cli tests: https://github.com/npm/cli/blob/9a564a55c016642bff3e4ae58ed05a0d6370aaf3/test/tap/deprecate.js

The only coverage that's missing is the Deprecate.builder function, which I'm not sure how to test.

otplease also needs to be tested.

@zkat
Copy link
Contributor

zkat commented Nov 12, 2018

This is amazing, thank you! Consider this command done! 🎉

@zkat zkat merged commit be8735c into npm:latest Nov 12, 2018
@noelleleigh noelleleigh deleted the add-deprecate branch November 12, 2018 18:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants