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

fix: make prefixed usage errors more consistent #3987

Merged
merged 1 commit into from Nov 4, 2021
Merged

Conversation

lukekarrys
Copy link
Member

No description provided.

@lukekarrys lukekarrys requested a review from a team as a code owner November 3, 2021 21:55
lib/commands/cache.js Outdated Show resolved Hide resolved
lib/commands/cache.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

The reason the tests didn't fail here is because the t.rejects in the tests are only testing for the presence of certain strings in the raised exceptions. And in fact I think there's a bug in t.rejects if you pass it a string it ... just passes no matter what

~/D/n/cli (PR-3987|✔) $ tap test/lib/commands/cache.js 
test/lib/commands/cache.js .......................... 28/28
total ............................................... 28/28

  28 passing (246.797ms)

  ok
~/D/n/cli (PR-3987|✚1) $ git diff
diff --git a/test/lib/commands/cache.js b/test/lib/commands/cache.js
index c12318f4e..e73a7cd3d 100644
--- a/test/lib/commands/cache.js
+++ b/test/lib/commands/cache.js
@@ -161,7 +161,7 @@ const cache = new Cache(npm)
 t.test('cache no args', async t => {
   await t.rejects(
     cache.exec([]),
-    'usage instructions',
+    'asdf',
     'should throw usage instructions'
   )
 })

@wraithgar wraithgar self-requested a review November 4, 2021 14:19
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

Need at least to remove this.usage from the function call. Fixing tests may be bigger than this PR since it affects more than just this one command.

@lukekarrys
Copy link
Member Author

lukekarrys commented Nov 4, 2021

And in fact I think there's a bug in t.rejects if you pass it a string it ... just passes no matter what

If the second parameter to t.rejects is a string, then that is treated as the message:

From the docs:

t.rejects(promise | fn, [expectedError], message, extra)

I looked up the test in libtap to make sure: https://github.com/tapjs/libtap/blob/66c23ee844d28ff144c502f7be432b7141c1343c/test/test.js#L473-L475

@lukekarrys lukekarrys marked this pull request as draft November 4, 2021 16:03
@lukekarrys lukekarrys changed the title chore: use usageError in more places fix: make prefixed usage errors more consistent Nov 4, 2021
@lukekarrys lukekarrys marked this pull request as ready for review November 4, 2021 17:54
log.silly('cache add', 'args', args)
if (args.length === 0)
throw Object.assign(new Error(usage), { code: 'EUSAGE' })
throw this.usageError('First argument to `add` is required')
Copy link
Member Author

Choose a reason for hiding this comment

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

I opted to change this error message to be more consistent with other commands.

Here's a diff of the error messages since we aren't capturing it in a snapshot:

diff --git a/oldoutput.log b/newoutput.log
index 974aaaa34..9693e7641 100644
--- a/oldoutput.log
+++ b/newoutput.log
@@ -1,10 +1,25 @@
 npm ERR! code EUSAGE
+npm ERR! 
+npm ERR! Usage: First argument to `add` is required
+npm ERR! 
+npm ERR! npm cache
+npm ERR! 
+npm ERR! Manipulates packages cache
+npm ERR! 
 npm ERR! Usage:
-npm ERR!     npm cache add <tarball-url>...
-npm ERR!     npm cache add <pkg>@<ver>...
-npm ERR!     npm cache add <tarball>...
-npm ERR!     npm cache add <folder>...
+npm ERR! npm cache add <tarball file>
+npm ERR! npm cache add <folder>
+npm ERR! npm cache add <tarball url>
+npm ERR! npm cache add <git url>
+npm ERR! npm cache add <name>@<version>
+npm ERR! npm cache clean [<key>]
+npm ERR! npm cache ls [<name>@<version>]
+npm ERR! npm cache verify
+npm ERR! 
+npm ERR! Options:
+npm ERR! [--cache <cache>]
 npm ERR! 
+npm ERR! Run "npm help cache" for more info

@wraithgar
Copy link
Member

I asked @lukekarrys permission to push a rather bold update to this PR, he can revert it at his whim. Usage output shouldn't look so scary. Now that we're standardizing on the error thrown we can handle it uniformly. This shows the before and after.

Before:
Screen Shot 2021-11-04 at 12 01 26 PM

After:
Screen Shot 2021-11-04 at 12 01 34 PM

Exit code is still 1 so this isn't a breaking change.

@wraithgar
Copy link
Member

After discussion we'll revert my change and implement it later, it at least needs to go to stdout. At least all usage is standardized so it'll be easier to implement when we're ready

@lukekarrys lukekarrys merged commit 22230ef into release-next Nov 4, 2021
@lukekarrys lukekarrys deleted the lk/eusage branch November 4, 2021 20:09
@wraithgar wraithgar mentioned this pull request Nov 4, 2021
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.

None yet

2 participants