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

Pretty Terminal Output #2787

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

Shivansh-007
Copy link
Contributor

@Shivansh-007 Shivansh-007 commented Jan 20, 2022

This introduces a style helper for styling your logs to get a bit pretty output in the terminal.

It adds an OutputLevels enum which can be used to define the level of the message "log" you are outputting. You can still use the old manual styles by passing fg, bold, dim, etc. individually to the function black.output.out(), but if they are specified alongside with style= kwarg (which specifies the OutputLevels) the manual styles would over-write the style specified by the style= kwarg. An example use case is:

out(
	error_msg if report.return_code else "All done! ✨ 🍰 ✨",
	style=OutputLevels.success,
	bold=False,
)
# The final style would be {"fg": "green", "bold": False} 
# (as we are overwriting it by manually passing `bold=False`

The current terminal output looks like this:

Screenshot from 2022-01-20 15-35-17

This PR needs discussion regarding the colour styling, currently, I just picked up what I think would be okay for the level.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation? -> n/a

This enum can be used with `black.out.output()` to create pretty output logs, backward compatibility is ensured and if both, manual styles and `style=` kwarg is passed, the manually passed styles would overwrite the styles of the `style=`.
@github-actions
Copy link

github-actions bot commented Jan 20, 2022

diff-shades reports zero changes comparing this PR (70c77ee) to main (4ea75cd).


What is this? | Workflow run | diff-shades documentation

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

All in all, I like the change especially for the easy access to predefined styles. Some comments below.



class OutputLevels(Enum):
null: Any = dict()
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed, I prefer literals. This way the flake8 config doesn't have to change as well. I'll include it here as well so that others may comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also none could be a more pythonic name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None would be more pythonic agreed, but I would need to add some additional logic to _out function, I wanted to keep it simple hence using am using an empty dict here.

Regarding the dict literals, I am using function calls here as I found them more readable compared to literals.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant none: dict = {}!

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should either use dict literals here or decide that we globally don't care for this flake8 setting and disable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant none: dict = {}!

Ah, that would work 👍🏻 (the name, mypy complains about : dict as it needs the parameters)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Flake8 rule is definitely useful. So maybe we'll sacrifice a bit of readability and use literals here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, either way is fine. We could even safely ignore the rule since very few people use functional calls over literals for normal use cases (which can be caught in reviews), otherwise, they are only used for some specific reasons like readability/explicitness (this case).

src/black/output.py Show resolved Hide resolved
**_styles: Any,
) -> None:
styles = style.value.copy()
if style:
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me this is redundant because .update with an empty dict does nothing. Also discussed before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but I wanted to keep the .update() change explicit hence keeping it in an if block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong, doesn't it mean we ignore _styles if style == LogLevel.none?

Also, the variable names style, styles, and _styles here are pretty confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I forgot to make that change.

Screenshot from 2022-01-22 10-43-07

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umm, can't think of a better name for _styles, these are the "styles" which the user explicitly passes as kwargs to overwrite the style defined by the LogLevel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest style_overrides.

src/black/output.py Outdated Show resolved Hide resolved
@@ -55,7 +62,7 @@ def failed(self, src: Path, message: str) -> None:

def path_ignored(self, path: Path, message: str) -> None:
if self.verbose:
out(f"{path} ignored: {message}", bold=False)
out(f"{path} ignored: {message}", style=OutputLevels.warning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this should be some other level, like info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to highlight them, I could use dim yellow though, by changing to OutputLevels.info and fg="yellow".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason as to why you'd want to highlight them? Don't get me wrong, it could be nice. But this is why we define the styles right? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted them to be easily recognizable from the crowd of "wasn't modified logs".

src/black/const.py Outdated Show resolved Hide resolved
@felix-hilden felix-hilden added the skip news Pull requests that don't need a changelog entry. label Jan 20, 2022
@Shivansh-007
Copy link
Contributor Author

I don't think a skip news label is needed, I just haven't added the changelog yet. This change will be affecting the user side of the project, so a changelog would be good 👍🏻

@felix-hilden felix-hilden removed the skip news Pull requests that don't need a changelog entry. label Jan 20, 2022
When we do `from black.output import out`, we get a object `out` pointing to function `black.output.out` function. But now when we _assign_ a new function to it via mock patching on `black.output.out`, the `out` object we had earlier still points to `black.output.out` (the old function, before patching). This is why when you are patching `black.output.out`, it wouldn"t be patched correctly, resulting in these errors.

Nedbat has got a better explaination at https://nedbatchelder.com/blog/201908/why_your_mock_doesnt_work.html, in case this was a bouncer to you.
Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

Looking better, but let's address all the comments!

src/black/const.py Show resolved Hide resolved
@JelleZijlstra
Copy link
Collaborator

Screen Shot 2022-01-21 at 9 12 56 PM
Here's how it looks in my terminal (top is this PR, bottom is current main).

Feedback:

  • We need to worry about making this readable for everyone, which is hard because there are so many different configurations. In your screenshot the "wasn't modified" lines are kind of hard to read.
  • The stuff at the top in blue is easier to read on current main where it's bolded.
  • I like the color difference between the ignored and well-formatted lines on your branch.

@Shivansh-007
Copy link
Contributor Author

Shivansh-007 commented Jan 22, 2022

That's a lot of bright green 👀 Anyways, regarding your second point, do you want to make all info level logs bold blue or just those? I would go with the former.

And regarding the third point, I don't dim them so that they are highlighted for the user, do you think the colour difference is enough or the non-dimness helps?

@JelleZijlstra
Copy link
Collaborator

I don't think you should change anything just yet, would be better to get more feedback first.

@JelleZijlstra
Copy link
Collaborator

This now has heavy merge conflicts. Should we still pursue this change?

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

4 participants