Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix logging #1590

Merged
merged 6 commits into from
Jul 13, 2016
Merged

Fix logging #1590

merged 6 commits into from
Jul 13, 2016

Conversation

gavofyork
Copy link
Contributor

  • Fixes problematic colour-codes in the web interface.
  • --log-file for redirecting logging to a file.

@gavofyork gavofyork added the A0-pleasereview 🤓 Pull request needs code review. label Jul 11, 2016
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 12, 2016
if let &Some(ref f) = &file {
// ignore errors - there's nothing we can do
let _ = f.locked().write_all(removed_color.as_bytes());
let _ = f.locked().write_all(b"\n");
Copy link
Contributor

@rphmeier rphmeier Jul 12, 2016

Choose a reason for hiding this comment

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

why lock twice?

there is also an implementation of write for &'a File, meaning you could probably avoid the mutex altogether and just pass a file handle

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 12, 2016
@rphmeier
Copy link
Contributor

lgtm other than mutex grumble

@arkpar
Copy link
Collaborator

arkpar commented Jul 12, 2016

Would be nice to disable colours for non-terminal output. Can be checked with libc::isatty
There's also https://github.com/dtolnay/isatty

@gavofyork
Copy link
Contributor Author

Would be nice to disable colours for non-terminal output

another PR i think

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 12, 2016
@rphmeier
Copy link
Contributor

rphmeier commented Jul 12, 2016

still unnecessarily uses a mutex. i have a commit which fixes this at rphmeier@7336442 which i can push to this branch as well

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 12, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 12, 2016
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 12, 2016
@gavofyork gavofyork merged commit 3abe3e1 into master Jul 13, 2016
@gavofyork gavofyork deleted the fix-logging branch July 13, 2016 07:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants