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

[colorize_ansi] Remove the possibility to use anything else than a MessageStyle #8412

Merged

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring
βœ“ πŸ“œ Docs

Description

I think we could be a lot more liberal with the breaking change for small internal things like that.

@Pierre-Sassoulas Pierre-Sassoulas added Breaking changes for 3.0 🦀 Maintenance Discussion or action around maintaining pylint or the dev workflow labels Mar 9, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0a6 milestone Mar 9, 2023
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #8412 (f591319) into main (d04734d) will increase coverage by 0.02%.
The diff coverage is 75.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8412      +/-   ##
==========================================
+ Coverage   95.69%   95.71%   +0.02%     
==========================================
  Files         175      175              
  Lines       18454    18451       -3     
==========================================
+ Hits        17659    17660       +1     
+ Misses        795      791       -4     
Impacted Files Coverage Ξ”
pylint/reporters/text.py 89.70% <75.00%> (+2.65%) ⬆️

@@ -37,6 +37,24 @@ class MessageStyle(NamedTuple):
style: tuple[str, ...] = ()
"""Tuple of style strings (see `ANSI_COLORS` for available values)."""

def get_ansi_code(self) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this private

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a little complicated to make a public class function private (then we need to call it with .__MessageStyle_ ... I'm wondering if we could use colorama entirely instead. Or make this a public API, it's unlikely it's ever going away.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand your comment. We can just make the function private and keep MessageStyle public right? We have similar patterns for all of our other classes: the class itself is public but not all method are public.

Copy link
Member Author

Choose a reason for hiding this comment

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

>>> class MessageStyle:
...     def __ansi_color(self):
...         print("ansi")
... 
>>> m = MessageStyle()
>>> m.__ansi_color()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'MessageStyle' object has no attribute '__ansi_color'
>>> m._MessageStyle__ansi_color()
ansi

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 _get_ansi_code. That signals that it is private and not part of MessageStyle's API

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha ok, I call that 'protected'.

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Mar 9, 2023

Choose a reason for hiding this comment

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

Well it could be made private with a small refactor f591319

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit f591319

@Pierre-Sassoulas Pierre-Sassoulas merged commit 2b3d113 into pylint-dev:main Mar 9, 2023
@Pierre-Sassoulas Pierre-Sassoulas deleted the colorize-ansi-deprecation branch March 9, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking changes for 3.0 🦀 Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants