Handle incorrect argument, print output file locations #149

honzajavorek · 2018-01-24T09:00:36Z

Introducing following changes:

bb dep check non-existing-package doesn't crash anymore
bb dep check package@non-existing-version doesn't crash anymore
The bb dep check command prints out locations where the output files are saved

Before:

$ bb dep check chromedriver
Analyzing the package...
Getting dependencies...
Creating the list of 4th party deps...
Creating the Public License field contents...

After:

$ bb dep check chromedriver
Analyzing the package...
Getting dependencies...
Creating the list of 4th party deps... /Users/honza/Projects/black-belt/list.txt
Creating the Public License field contents... /Users/honza/Projects/black-belt/licenses.txt

abtris

LGTM

abtris · 2018-01-24T12:25:21Z

Need rebase with master

Almad

I'd either make less granular exception checking or print stderr in error messages, otherwise this will make very confusing error outputs.

Almad · 2018-01-24T14:39:15Z

blackbelt/commands/dep.py

-            dep_version = run(['npm', 'view', dep_name, 'version'])
+            try:
+                dep_version = run(['npm', 'view', dep_name, 'version'])
+            except subprocess.CalledProcessError:


I think it would be good to look into exit codes / status messages, because this wraps multiple errors under one hood and can make it very tricky to debug (i.e. if there is any other general npm or network problem).

Perhaps it would be simpler to use the NPM web API directly, instead of invoking the CLI tool and trying to interpret it and it's response.

Can simply do a GET request to get the package information https://api.npms.io/v2/package/dredd and look for version in there. 404 response would indicate missing package.

Python one liner, can be broken into more for a response.raise_for_status():

>>> requests.get('https://api.npms.io/v2/package/dredd').json()['collected']['metadata']['version'] '4.9.3'

https://api-docs.npms.io

@Almad Yup, my solution is a hotfix.
@kylef That's an amazing finding/an idea. I support you mentally in sending a PR! 😏

Almad · 2018-01-24T14:40:07Z

blackbelt/dependencies.py

+        try:
+            install(dep_name, dep_version, tmp_dir, dev=dev)
+        except subprocess.CalledProcessError:
+            raise click.BadParameter('The npm package could not be installed')


As above, in this case, I'd do ...installed: ', stderr)

Okay, passing the error through might be worth.

honzajavorek · 2018-01-24T17:16:49Z

Not sure when I'll be able to get back to this tho. Feel free to jump in and cherry-pick or to bother me to finalize this. It has zero priority for me right now.

honzajavorek · 2018-01-29T09:43:09Z

@kylef @Almad Updated

abtris · 2018-02-19T08:44:05Z

@Almad can you make re-review we have 21 days old last comment

kylef · 2018-02-19T11:30:37Z

blackbelt/commands/dep.py

+                else:
+                    if ctx.params.get('debug'):
+                        raise
+                    raise click.BadParameter('Unable to figure out the package version')


I don't think this is semantically correct, the error is not a "BadParameter" but some HTTP/DNS or similar problem.

I'll change it to click.Abort

There has been changes made.

honzajavorek · 2018-02-21T15:36:16Z

@kylef I somehow managed to dismiss your review 😞

Almad

I'd add more into the message for diagnosis

Almad · 2018-03-27T20:45:37Z

blackbelt/dependencies.py

+        except Exception as e:
+            if debug:
+                raise
+            raise click.BadParameter('The npm package could not be installed')


@abtris wouldn't it make sense to include the e.message?

Almad · 2018-03-27T20:46:08Z

blackbelt/commands/dep.py

+                else:
+                    if ctx.params.get('debug'):
+                        raise
+                    raise click.Abort('Unable to figure out the package version')


I'd say "why"—at least a status_code or pick something sensible from the response

honzajavorek requested review from kylef, Almad and abtris January 24, 2018 09:01

abtris previously approved these changes Jan 24, 2018

View reviewed changes

Almad previously requested changes Jan 24, 2018

View reviewed changes

honzajavorek dismissed abtris’s stale review via 33fa172 January 29, 2018 09:42

honzajavorek added 2 commits January 29, 2018 10:42

fix: Handle incorrect argument, print output file locations

c6b820c

fix: Allow user to see the stack traces with --debug

96ca12c

honzajavorek force-pushed the honzajavorek/print-output-file-locations branch from 33fa172 to 96ca12c Compare January 29, 2018 09:42

kylef previously approved these changes Feb 19, 2018

View reviewed changes

kylef reviewed Feb 19, 2018

View reviewed changes

fix: Use proper exception type

578c026

honzajavorek dismissed kylef’s stale review via 578c026 February 21, 2018 15:34

kylef approved these changes Feb 21, 2018

View reviewed changes

Merge branch 'master' into honzajavorek/print-output-file-locations

6364f93

abtris merged commit 1aad65f into master Feb 27, 2018

abtris deleted the honzajavorek/print-output-file-locations branch February 27, 2018 10:41

Almad reviewed Mar 27, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle incorrect argument, print output file locations #149

Handle incorrect argument, print output file locations #149

honzajavorek commented Jan 24, 2018

abtris left a comment

abtris commented Jan 24, 2018 •

edited

Loading

Almad left a comment

Almad Jan 24, 2018

kylef Jan 24, 2018 •

edited

Loading

honzajavorek Jan 24, 2018

Almad Jan 24, 2018

honzajavorek Jan 24, 2018

honzajavorek commented Jan 24, 2018

honzajavorek commented Jan 29, 2018

abtris commented Feb 19, 2018

kylef Feb 19, 2018 •

edited

Loading

honzajavorek Feb 21, 2018

honzajavorek commented Feb 21, 2018

Almad left a comment

Almad Mar 27, 2018

Almad Mar 27, 2018

Handle incorrect argument, print output file locations #149

Handle incorrect argument, print output file locations #149

Conversation

honzajavorek commented Jan 24, 2018

abtris left a comment

Choose a reason for hiding this comment

abtris commented Jan 24, 2018 • edited Loading

Almad left a comment

Choose a reason for hiding this comment

Almad Jan 24, 2018

Choose a reason for hiding this comment

kylef Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

honzajavorek Jan 24, 2018

Choose a reason for hiding this comment

Almad Jan 24, 2018

Choose a reason for hiding this comment

honzajavorek Jan 24, 2018

Choose a reason for hiding this comment

honzajavorek commented Jan 24, 2018

honzajavorek commented Jan 29, 2018

abtris commented Feb 19, 2018

kylef Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

honzajavorek Feb 21, 2018

Choose a reason for hiding this comment

honzajavorek commented Feb 21, 2018

Almad left a comment

Choose a reason for hiding this comment

Almad Mar 27, 2018

Choose a reason for hiding this comment

Almad Mar 27, 2018

Choose a reason for hiding this comment

abtris commented Jan 24, 2018 •

edited

Loading

kylef Jan 24, 2018 •

edited

Loading

kylef Feb 19, 2018 •

edited

Loading