Skip to content

Better console conventions - decouple console format logic from output formatting, ensure stdout is for output only #473

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

Merged
merged 5 commits into from
Apr 9, 2020

Conversation

mariuszskon
Copy link
Contributor

As mentioned in #425 the console format does not make much sense because it is a CSV anyway.
This PR fixes this.
Furthermore, to make the console output more useful (in the spirit of #267), I have ensured that logging only goes to stderr instead of stdout so it is easier to do things in the shell e.g. pipe output of cve-bin-tool to grep.

@terriko
Copy link
Contributor

terriko commented Mar 17, 2020

Hm, I don't know if we actually want the log.info stuff to go only to stderr. I'll have to think about this a bit.

@mariuszskon
Copy link
Contributor Author

Hi @terriko , to try to persuade you 😃 I compiled some links, which agree that all sorts of diagnostics (logs, including non-errors) should go to stderr:
https://www.gnu.org/software/libc/manual/html_node/Standard-Streams.html
https://pubs.opengroup.org/onlinepubs/9699919799/functions/stderr.html
https://linux.die.net/man/3/stderr

There are also some which disagree or do not clearly agree:
https://docs.python.org/3/library/sys.html#sys.stderr
ipfs/go-log#60

@terriko
Copy link
Contributor

terriko commented Mar 30, 2020

Thank you! Alas, the problem is not that I'm unconvinced so much as that I don't have time to sit and properly make a decision. Between my regular job, GSoC, and childcare I'm basically working 3 jobs at once right now, plus now PyCon wants us to upload videos of our talks since the conference itself is cancelled. Since this PR isn't as urgent, it's probably going to wait until at least GSoC and PyCon have quieted down for me.

@johnandersen777
Copy link

Has this been updated to work with a3afc9e?

@johnandersen777 johnandersen777 added the awaiting submitter Need more information from submitter label Apr 6, 2020
@mariuszskon
Copy link
Contributor Author

@pdxjohnny no it has not. I'll have a closer look soon, but a quick glance shows that only the "console" choice will need to be lightly amended, and any documentation.

@mariuszskon
Copy link
Contributor Author

@pdxjohnny I have implemented the changes which I believe make this PR make sense with a3afc9e.

@codecov-io
Copy link

Codecov Report

Merging #473 into master will decrease coverage by 0.13%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
- Coverage   86.52%   86.38%   -0.14%     
==========================================
  Files          60       60              
  Lines        2411     2409       -2     
  Branches      488      489       +1     
==========================================
- Hits         2086     2081       -5     
- Misses        212      214       +2     
- Partials      113      114       +1     
Flag Coverage Δ
#longtests 85.00% <71.42%> (-0.14%) ⬇️
#ubuntutests 74.56% <71.42%> (-0.15%) ⬇️
#wintests 41.23% <42.85%> (-0.26%) ⬇️
Impacted Files Coverage Δ
cve_bin_tool/cli.py 84.82% <50.00%> (+0.29%) ⬆️
cve_bin_tool/OutputEngine.py 70.65% <75.00%> (+0.19%) ⬆️
cve_bin_tool/log.py 81.25% <100.00%> (-18.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e672c2...f07427f. Read the comment docs.

@terriko
Copy link
Contributor

terriko commented Apr 9, 2020

Okay, finally got some time to think about it and look at the output that we currently have. I think you're right, stderr is the right place for all the messages, and it's especially obvious now that we have multiple output formats. Thanks so much for this, and sorry it took so long for me to give it the attention it deserved!

@terriko terriko merged commit e482793 into intel:master Apr 9, 2020
@mariuszskon
Copy link
Contributor Author

No worries at all, I say it was a very reasonable turnaround time 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter Need more information from submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants