[WIP] log: use bright/dim colors for ui#3835
Conversation
| ) | ||
|
|
||
|
|
||
| def styled(message, style=None): |
There was a problem hiding this comment.
@skshetry this awesome and long waited from my end ... the only question - how well it supported by older/SSH terminals, Windows, etc - we should do some fallback to regular colors? or will this logic handle this automatically?
(also, really wanted to improve the way we print the box around telemetry, updater messages - there was some concern with older terminals as well that prevented us from using nice symbols - may be we can detect it also)
There was a problem hiding this comment.
(I see that we only change brightness here, also as a good next step we can start using normal modern colors instead of RED, BLUE - etc that are even hard to read sometimes - again the biggest question is compatibility and fallbacks)
There was a problem hiding this comment.
It should work on Windows as it uses colorama, but not sure of others.
There was a problem hiding this comment.
I was using colors before, like I showed yesterday. But, I moved away from it, because if other log appears in between, mix of colored and non-colored output looks weird. Plus, the colors that work on both dark/light terminals need to be choosen (most probably, red/green/yellow).
There was a problem hiding this comment.
But, I moved away from it, because if other log appears in between, mix of colored and non-colored output looks weird.
do you mean when we use -v?
I was using colors before, like I showed yesterday.
Sorry, I was confusing. Your are right about using brightness for these logs. Everything else I mentioned is not directly related to this PR. So, please feel free to disregard :)
I was saying about using "pastel" terminal colors instead of regular RED, BLUE - e.g. https://github.com/tartley/colorama/blob/master/colorama/ansi.py#L60 - for all other logs. Same as nmp or heroku cli do - I think they can detect if terminal supports modern colors or not.
Also using some nice looking symbols for the box instead of a regular dash.
There was a problem hiding this comment.
do you mean when we use -v?
No. I mean, as we use INFO for ui, we can either support only one color (colored or not) or manually change all to whatever color we like. But, any logger.info call will not be colored, so we might end up with mixed output. To have a colored output, we'd constantly fighting with it.
Same as nmp or heroku cli do
I did gave a quick look to https://github.com/chalk/supports-color, looks like it's possible. But, do we really need True colors (i.e. 256 colors are enough for us? At least for a start).
There was a problem hiding this comment.
256 is totally enough. (again, not really related to this PR, I would suggest a ticket for this)
No. I mean, as we use INFO for ui, we can either support only one color (colored or not) or manually change all to whatever color we like. But, any logger.info call will not be colored, so we might end up with mixed output. To have a colored output, we'd constantly fighting with it.
not sure I completely follow ... but looks like another reason to not use logger for UI.
| data = parse_stage_for_update(fd.read(), self.path) | ||
| else: | ||
| logger.info( | ||
| "%s does not exist, creating…", styled(self.relpath, "bold") |
There was a problem hiding this comment.
Minor: Do we use … (ellipsis character) a lot? I'm guessing it's mostly ... (3 period chars) in other files, if any.
There was a problem hiding this comment.
Also
| "%s does not exist, creating…", styled(self.relpath, "bold") | |
| "'%s' does not exist, creating…", styled(self.relpath, "bold") |
| logger.warning( | ||
| '{stage} is a "callback" stage ' | ||
| logger.debug( | ||
| '%s is a "callback" stage ' |
There was a problem hiding this comment.
| '%s is a "callback" stage ' | |
| ''%s' is a "callback" stage ' |
| logger.info("%s changed.", self) | ||
| else: | ||
| logger.info("%s didn't change.", self) | ||
| logger.debug("%s changed.", self) |
There was a problem hiding this comment.
| logger.debug("%s changed.", self) | |
| logger.debug("'%s' changed.", self) |
| ) | ||
| if cached: | ||
| logger.info( | ||
| "Stage %s is cached, skipping…", styled(stage.addressing, "bold"), |
There was a problem hiding this comment.
| "Stage %s is cached, skipping…", styled(stage.addressing, "bold"), | |
| "Stage '%s' is cached, skipping…", styled(stage.addressing, "bold"), |
| restored = stage.outs_cached() | ||
| if restored: | ||
| logger.info( | ||
| "Restored stage %s from run-cache, skipping…", |
There was a problem hiding this comment.
| "Restored stage %s from run-cache, skipping…", | |
| "Restored stage '%s' from run-cache, skipping…", |
Is run-cache (hyphenated) the official term we will be using consistently?
| "{} ".format(styled("callback", "bold")) if stage.is_callback else "" | ||
| ) | ||
| logger.info( | ||
| "Running %s" "stage %s with command:", |
There was a problem hiding this comment.
| "Running %s" "stage %s with command:", | |
| "Running '%s'" "stage '%s' with command:", |
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
Left some questions and suggestions.
|
Will rework on it after #3850. Closing for now. |
3 participants
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
On top of #3834.