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

Resolve logging problem #231

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Conversation

ketkarameya
Copy link
Collaborator

@ketkarameya ketkarameya commented Sep 2, 2022

So since a long time, our logs were not getting printed properly :| I tried a lot hacks and work arounds but for some reason I am unable to have the logs written to a particular log file with color and has some encoding issue :-| Now there are some work arounds mentioned in the issues : 1 and 2, but it doesn't work for me :-| . Now we either use some other library (apparently the one we are using is most popular) or we just cat when we want logs to be written to a particular file.

I also cleaned up the logs -
(i) removed unnecessary logs
(ii) println -> info

@ketkarameya ketkarameya changed the base branch from master to feature/simple_python_bindings September 2, 2022 04:47
@ketkarameya ketkarameya marked this pull request as ready for review September 2, 2022 05:33
@ketkarameya ketkarameya changed the title Bugfix/logging Solve logging problem Sep 2, 2022
@ketkarameya ketkarameya changed the title Solve logging problem Resolve logging problem Sep 2, 2022
Copy link
Contributor

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

Minor nits about imports, but overall I am fine with this change.

Question: when you say you can't output logs in color to a file, do you mean that if you add calls like .green() the logs aren't written at all when sent to a file? Because if you just mean the color is lost, then I'd expect that from the fact that it's a plain test file and not a rich TTY.

@@ -14,7 +14,7 @@ Copyright (c) 2022 Uber Technologies, Inc.
use std::collections::HashMap;

use colored::Colorize;
use log::info;
use log::{info};
Copy link
Contributor

Choose a reason for hiding this comment

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

Needed? I think log::info is the right syntax if we are just importing that, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

YEs. Addressed by cargo format.

I think I ll add a new Github action that runs cargo format on each PR, as a commit. Idk

@@ -13,11 +13,13 @@ Copyright (c) 2022 Uber Technologies, Inc.

use std::path::{Path, PathBuf};

use log::{error};
Copy link
Contributor

Choose a reason for hiding this comment

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

use log::error

@@ -19,7 +19,7 @@ use crate::{
};
use colored::Colorize;
use itertools::Itertools;
use log::info;
use log::{info};
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something about log::{info} vs log::info

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No its equivalent.
I think during my changes I had added a warn macro in this file and made this import ->
log::info -> log::{info, warn}. But then I removed warn, and {} remained. I will clean i up.

@ketkarameya
Copy link
Collaborator Author

Minor nits about imports, but overall I am fine with this change.

Question: when you say you can't output logs in color to a file, do you mean that if you add calls like .green() the logs aren't written at all when sent to a file? Because if you just mean the color is lost, then I'd expect that from the fact that it's a plain test file and not a rich TTY.

Hmmm Its actually that the characters don't get encoded and are shown as blobs :| . It is open issue (rust-cli/env_logger#208 and rust-cli/env_logger#178) it has something to do with the pipe operation.

@ketkarameya ketkarameya merged commit be366d4 into feature/simple_python_bindings Sep 2, 2022
@ketkarameya ketkarameya deleted the bugfix/logging branch February 13, 2023 03:13
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

2 participants