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

use standard logging #303

Closed
anarcat opened this issue Oct 5, 2018 · 6 comments
Closed

use standard logging #303

anarcat opened this issue Oct 5, 2018 · 6 comments
Labels
Accepted Accepted issue on our roadmap Design Design or UX/UI related Sprintable Small enough to sprint on Stretch Optional goal for the current sprint (may not be delivered)
Milestone

Comments

@anarcat
Copy link
Contributor

anarcat commented Oct 5, 2018

whipper currently writes its error messages and output all jumbled up together on stdout, through the Program._stdout file handler. This is useful because it could allow some tests to parse the output of the program and verify things are working okay. That is not currently being used however, so it's unclear to me why this construct is present at all - the only test I could find that would use something like this TestAccurateRipReport and it directly hijacks sys.stdout and does not touch directly the Program._stdout handler.

So I would recommend switching to using the logging module. It's standard, well known and expected in Python programs. As a bonus, all messages will be written on stderr (as they should). logfile, syslog and other handlers support becomes trivial to implement as well. The conversion should be easy: just change:

self._stdout.write("foo %s\n", bar)

to:

logging.info("foo %s", bar)

or:

logging.warning....

Depending on the severity level.

How does that sound?

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 6, 2018

That is not currently being used however, so it's unclear to me why this construct is present at all

I think that instructions are a legacy of whipper's origin: morituri (and early versions of whipper) didn't use the Python logging module. They can be probably converted without regrets... To be sure, let's ask @RecursiveForest and @MerlijnWajer.

@MerlijnWajer
Copy link
Collaborator

Sounds good to me if we can get rid of the flog dependency.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 16, 2018

@anarcat 👌

@JoeLametta
Copy link
Collaborator

/remind me "to review this issue" in 12 hours

@reminders reminders bot added the Reminder User requested reminder: automatically tagged by the reminders bot label Nov 10, 2018
@reminders
Copy link

reminders bot commented Nov 10, 2018

@JoeLametta set a reminder for Nov 11th 2018

@JoeLametta JoeLametta added Status: in progress Issue/pull request which is currently being worked on Accepted Accepted issue on our roadmap Design Design or UX/UI related Sprintable Small enough to sprint on Stretch Optional goal for the current sprint (may not be delivered) and removed Reminder User requested reminder: automatically tagged by the reminders bot enhancement labels Nov 11, 2018
@JoeLametta JoeLametta added this to the 2.0 milestone Nov 13, 2018
@JoeLametta JoeLametta removed the Status: in progress Issue/pull request which is currently being worked on label Dec 14, 2018
@JoeLametta JoeLametta modified the milestones: 2.0, 1.0 Dec 15, 2018
@anarcat
Copy link
Contributor Author

anarcat commented Dec 17, 2018

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Design Design or UX/UI related Sprintable Small enough to sprint on Stretch Optional goal for the current sprint (may not be delivered)
Projects
None yet
Development

No branches or pull requests

3 participants