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

Adds a new list reporter #10

Closed
wants to merge 3 commits into from
Closed

Adds a new list reporter #10

wants to merge 3 commits into from

Conversation

luislobo
Copy link
Contributor

@luislobo luislobo commented May 9, 2018

Adds a new list reporter, with a more streamlined output.
Lets you do | grep critical (for example), as it prints everything in one line.

Please check the TODO comment to see if it's valid

I also have a change for npm that I'm about to send after this one, so that npm audit has a --list parameter

Adresses #7

… less than a page. Also lets you do `| grep critical` (for example)
@evilpacket
Copy link
Contributor

@luislobo I haven't had a chance to test this out, do you happen to have a screenshot of the output (I swear I've seen one someplace and thought it was here but now it's not so I'm confused) :)

@luislobo
Copy link
Contributor Author

luislobo commented May 10, 2018

yeah sure! It was on a tweet in response to @zkat tweet about audit.

dcxigkwxuaew9y0

@luislobo
Copy link
Contributor Author

@zkat suggested changing it as --parseable making it output the result like npm install --parseable, I think that both use cases are valid, the --list one and the --parseable one. If you agree, I can add also parseable

npm install --parseable outputs something like this:

add	abbrev	1.1.1	node_modules/fsevents/node_modules/abbrev		
add	ansi-regex	2.1.1	node_modules/fsevents/node_modules/ansi-regex		
add	aproba	1.2.0	node_modules/fsevents/node_modules/aproba		
add	balanced-match	1.0.0	node_modules/fsevents/node_modules/balanced-match		
add	chownr	1.0.1	node_modules/fsevents/node_modules/chownr		
add	code-point-at	1.1.0	node_modules/fsevents/node_modules/code-point-at		
add	concat-map	0.0.1	node_modules/fsevents/node_modules/concat-map		
add	brace-expansion	1.1.11	node_modules/fsevents/node_modules/brace-expansion		
add	console-control-strings	1.1.0	node_modules/fsevents/node_modules/console-control-strings		
add	core-util-is	1.0.2	node_modules/fsevents/node_modules/core-util-is		

@zkat zkat closed this May 10, 2018
@luislobo
Copy link
Contributor Author

@zkat may I ask why it was closed?

@zkat
Copy link
Contributor

zkat commented May 10, 2018

oh. I guess this happened because of renaming the default branch to latest.

@zkat zkat reopened this May 10, 2018
@zkat zkat changed the base branch from master to latest May 10, 2018 20:44
@zkat
Copy link
Contributor

zkat commented May 10, 2018

fixed :)

@luislobo
Copy link
Contributor Author

Awesome 😍

@zkat
Copy link
Contributor

zkat commented May 11, 2018

I'm gonna /cc @iarna and @evilpacket on this to get their perspective on the --list view. I'm neutral on it and would still like a --parseable reporter (and I still think --list could be replaced with a fairly readable version of --parseable).

@evilpacket
Copy link
Contributor

My initial concern about the screenshot above is terminal width requirements and trying to keep output , but I'm pro a greppable report, which could probably meet both requirements with a --parsable style report.

@luislobo
Copy link
Contributor Author

luislobo commented May 12, 2018

I could make a --parseable version with colors, as I've seen that colors can be enabled/disabled too. We could add a param for --no-color if there is a need (didn't check yet how disabling the colors works internally)

I'm fine with getting rid of the --list option.

One question though, it's a funcional one. Can you check the PR, there is a TODO/TOCHECK here: https://github.com/npm/npm-audit-report/pull/10/files#diff-a06ff6c66ce9794c6a36389e432a5266R117

@iarna
Copy link
Contributor

iarna commented May 15, 2018

npm cli side, parseable output should never have colors (it is solely intended for consumption by tooling and should not require additional flags).

Copy link
Contributor

@iarna iarna left a comment

Choose a reason for hiding this comment

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

Before this can land tests will need to be passing (currently this is failing our linter)

@luislobo luislobo mentioned this pull request May 21, 2018
@luislobo
Copy link
Contributor Author

Replaced this PR with #21, closing this one.

New one has --parseable, has no colors, passes lint and enhanced tests.

@luislobo luislobo closed this May 21, 2018
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

4 participants