Skip to content

Make colour aliases IDE-friendly #2050

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

Merged
merged 4 commits into from
Oct 2, 2021
Merged

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Sep 15, 2021

Fixes #2049

Overview: What does this pull request change?

Makes colour aliases like BLUE more tool friendly (e.g. in VSCode).

Motivation and Explanation: Why and how do your changes improve the library?

See #2049

Links to added or changed documentation pages

Further Information and Comments

No idea why the grey order was A, B, C, E, D in the old code lol.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@GameDungeon
Copy link
Contributor

While I wholeheartedly agree with this change, some other tried to fight it in the past for reasons I've now forgotten.

@vvolhejn
Copy link
Contributor

I like this! I wish manim was less magical in other places as well (mobject.animate mostly)

@Timmmm
Copy link
Contributor Author

Timmmm commented Sep 17, 2021

Yeah 100% agree about animate. Hopefully we can just add manual type annotations for those magically generated methods.

@Timmmm
Copy link
Contributor Author

Timmmm commented Sep 22, 2021

Maybe I'm missing something but why does this still say Review required but GameDungion has approved it?

@GameDungeon
Copy link
Contributor

I'm not an admin, it needs at least one person with commit privileges. That person takes the other reviewers into account.

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Eventually, I would like to improve our cluttered namespace and, for example, make people use colors from an enum. But well, this is not today, and the change here is a fine quality of life improvement (and it is not like we change the available colors very often).

I'll approve this, and if there are no concerns until later today, I'll merge it so that it is included in the upcoming release.

Thank you for your contribution!

@Timmmm

This comment has been minimized.

@Timmmm
Copy link
Contributor Author

Timmmm commented Oct 2, 2021

Wait, ignore my last comment; thought we were on a different PR. This one is good to go!

@behackl behackl enabled auto-merge (squash) October 2, 2021 09:52
@behackl behackl merged commit dd16347 into ManimCommunity:main Oct 2, 2021
@behackl behackl added the maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements label Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colour constants are not tool friendly.
5 participants