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

Add --color-palette option to pyreverse #8223

Merged
merged 4 commits into from
Feb 9, 2023

Conversation

DudeNr33
Copy link
Collaborator

@DudeNr33 DudeNr33 commented Feb 7, 2023

Type of Changes

Type
✨ New feature
🔨 Refactoring

Description

  • Add support for user-defined color palettes
  • Use a new default color palette, that is (hopefully) more visually pleasing and more accessible for color blind people. This palette is pretty close to what seaborn uses in their colorblind palette (see here)

The new default colors look like this:
image

Closes #6738

@DudeNr33 DudeNr33 added Enhancement ✨ Improvement to a component pyreverse Related to pyreverse component labels Feb 7, 2023
@DudeNr33 DudeNr33 added this to the 2.17.0 milestone Feb 7, 2023
@DudeNr33 DudeNr33 self-assigned this Feb 7, 2023
@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Merging #8223 (64e68b5) into main (4cbabc9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8223   +/-   ##
=======================================
  Coverage   95.45%   95.45%           
=======================================
  Files         177      177           
  Lines       18635    18661   +26     
=======================================
+ Hits        17788    17813   +25     
- Misses        847      848    +1     
Impacted Files Coverage Δ
pylint/pyreverse/main.py 93.02% <100.00%> (+0.16%) ⬆️
pylint/pyreverse/writer.py 100.00% <100.00%> (ø)
pylint/testutils/pyreverse.py 98.07% <100.00%> (+0.07%) ⬆️
pylint/checkers/variables.py 97.29% <0.00%> (-0.09%) ⬇️
pylint/checkers/nested_min_max.py 100.00% <0.00%> (ø)
pylint/checkers/refactoring/refactoring_checker.py 98.33% <0.00%> (ø)
pylint/checkers/imports.py 94.33% <0.00%> (+0.04%) ⬆️
pylint/checkers/utils.py 95.83% <0.00%> (+0.06%) ⬆️

nickdrozd
nickdrozd previously approved these changes Feb 7, 2023
Copy link
Contributor

@nickdrozd nickdrozd left a comment

Choose a reason for hiding this comment

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

Defaults looks great. Tested input color flag; works. 🎨

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.

Maybe we should keep what we had because it's a kind of breaking change. I'm so not the person to tell you if the default color palette looks good or not... But now that it can be user defined then it's fine, user can change it to taste :)

@nickdrozd
Copy link
Contributor

Maybe we should keep what we had because it's a kind of breaking change. I'm so not the person to tell you if the default color palette looks good or not... But now that it can be user defined then it's fine, user can change it to taste :)

By that same logic, anyone who just has to have alice blue and antique white is also free to specify them! But somehow I have the feeling that few will actually choose those colors 😆

@Pierre-Sassoulas
Copy link
Member

You'd be surprised, people get accustomed to thing and if it changes there's always someone complaining. https://imgs.xkcd.com/comics/workflow.png

@github-actions

This comment has been minimized.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Feb 8, 2023

I don't have a strong opinion on changing the default colors.
We can leave them as they are, and change them in 3.0.

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'd prefer to minimize churn but I'm also ok if @nickdrozd feels strongly about changing the default :)

@github-actions

This comment has been minimized.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Feb 9, 2023

I will leave it as it is currently (i.e. stick with the previous default colors and leave the TODO to switch for 3.0) and will take care of the spelling errors this evening.
Trying to install enchant yesterday resulted in a downwards spiral that made me update macOS. Sometimes you gotta love being a developer... 😅

@Pierre-Sassoulas
Copy link
Member

Trying to install enchant yesterday resulted in a downwards spiral that made me update macOS. Sometimes you gotta love being a developer...

https://www.youtube.com/watch?v=AbSehcT19u0

@nickdrozd
Copy link
Contributor

Sure, the default colors could be changed later, as part of the wider push for sane defaults. But they really should be changed. It's not just my opinion that default colors are bad; the SO post that prompted the color issue specifically mentions wanting to change the "faint blue color" to a darker one.

And it's not just about opinions. The new proposed colors are objectively better in terms of accessibility than the current defaults. Actually, the ugliness and the inaccessibility have the same root cause, which is that alice blue and antique white are both difficult to distinguish from each other and difficult to distinguish from a white background.

As an example, here is a package diagram generated by Pyreverse with the current colors:

packages

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Feb 9, 2023

As a 3.0 version is probably not far away from what I understood (#7607 (comment)), let's not do a breaking change in 2.17 and just do it with the next major version.

Also as a side note, pyenchant with Apple Silicon is just a big oof. You need to install both Python and enchant as x86_64 version (which is possible by following their docs; you will afterwards find yourself impossible to run pylint ever again with the native arm64 version of Python, as it will always try to load the incompatible x86 binaries.

So I'll still have to rely on the CI for catching spelling problems. 😐

@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) February 9, 2023 19:12
@Pierre-Sassoulas Pierre-Sassoulas changed the title Add --color-palette option to pyreverse and modify default colors Add --color-palette option to pyreverse Feb 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 64e68b5

@Pierre-Sassoulas Pierre-Sassoulas merged commit f9d796f into pylint-dev:main Feb 9, 2023
@DudeNr33 DudeNr33 deleted the pyreverse-color-palettes branch February 9, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component pyreverse Related to pyreverse component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom colour palettes for pyreverse
3 participants