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.

Sign up for GitHub

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

Jump to bottom

Add typing to text reporter #5041

Merged
merged 9 commits into from
Sep 20, 2021
Merged

Add typing to text reporter #5041

merged 9 commits into from
Sep 20, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
✓ 🔨 Refactoring

Description

Section of #4954

@DanielNoord DanielNoord mentioned this pull request Sep 18, 2021
Closed
2 tasks
@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 18, 2021
DanielNoord
DanielNoord commented Sep 18, 2021
View reviewed changes
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
cdce8p
cdce8p reviewed Sep 18, 2021
View reviewed changes
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
cdce8p
cdce8p reviewed Sep 18, 2021
View reviewed changes
pylint/reporters/text.py Outdated Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

Because of the refactor of MessageStyle the diff has become quite large. Sorry about that. If you want I can still split, but since Marc already did a first review of the earlier commits I think this should be good.

cdce8p
cdce8p requested changes Sep 18, 2021
View reviewed changes
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

First set of comments. There is some more work required to preserve backwards compatibility. Will leave some more comments for that.

pylint/reporters/base_reporter.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Show resolved Hide resolved
pylint/reporters/text.py Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
Pierre-Sassoulas
Pierre-Sassoulas reviewed Sep 18, 2021
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I never saw color working in pylint, so I wonder if this code is even working in the first place. It makes me want to add colorama to the dependency and refactor.

cdce8p
cdce8p reviewed Sep 18, 2021
View reviewed changes
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

To preserve backwards compatibility, these changes would be needed.

pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

I never saw color working in pylint, so I wonder if this code is even working in the first place. It makes me want to add colorama to the dependency and refactor.

Does @cdce8p know anything about this?

@cdce8p
Copy link
Member

cdce8p commented Sep 19, 2021

I never saw color working in pylint, so I wonder if this code is even working in the first place. It makes me want to add colorama to the dependency and refactor.

Does @cdce8p know anything about this?

Needed to check myself as I only use the default output. The color output does seem to work when configured

pylint --output-format colorized

cdce8p
cdce8p reviewed Sep 19, 2021
View reviewed changes
pylint/reporters/text.py Outdated Show resolved Hide resolved
cdce8p
cdce8p reviewed Sep 20, 2021
View reviewed changes
pylint/reporters/text.py Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Outdated Show resolved Hide resolved
pylint/reporters/text.py Show resolved Hide resolved
pylint/reporters/text.py Show resolved Hide resolved
cdce8p
cdce8p reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Almost there. One last comment

pylint/reporters/text.py Outdated Show resolved Hide resolved
@DanielNoord @cdce8p
Update pylint/reporters/text.py
16ffb2b 
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
cdce8p
cdce8p approved these changes Sep 20, 2021
View reviewed changes
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Looks good!
Thanks @DanielNoord 🚀

@DanielNoord
Copy link
Collaborator Author

@cdce8p Thanks for the review 😄

@DanielNoord DanielNoord merged commit b3336c7 into pylint-dev:main Sep 20, 2021
@DanielNoord DanielNoord deleted the typing-text-reporter branch September 20, 2021 22:05
@DanielNoord
Copy link
Collaborator Author

I'm so sorry, while squashing I removed the co-author line. Will be more cautious in the future!

@cdce8p
Copy link
Member

cdce8p commented Sep 20, 2021

I'm so sorry, while squashing I removed the co-author line. Will be more cautious in the future!

Don't worry about it! I'm already happy that you squash merged it.

@DanielNoord DanielNoord added this to the 2.11.2 milestone Sep 20, 2021
This was referenced Sep 20, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas left review comments

@cdce8p cdce8p cdce8p approved these changes

Assignees
No one assigned
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Milestone
2.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@DanielNoord @cdce8p @Pierre-Sassoulas