Skip to content

Conversation

@leotrs
Copy link
Contributor

@leotrs leotrs commented Sep 1, 2020

Context

This PR belongs to the ongoing refactor of the config system (#337).

Main change

The main goal here is to use the config dict as little as possible during library initialization. After this PR, config is only being used by a class/instance after the library has been imported (and thus after all other classes/functions/variables are already defined).

The motivation

The current state of the codebase contains a lot of CONFIG dictionaries as class attributes. Some of these have keys that are set using the value of the global config dict. Since all classes (and therefore all class attributes) are defined as soon as the library is being imported, all of the CONFIG attributes contain keys that are set to the values of config at the time when the library is first imported. This means that if a user/dev tries to change the config in the middle of a python session (for example, to perform tests), the CONFIG attributes will not reflect those changes. This is Bad. This PR changes the Bad to Good.

List of changes

  • Most of the changes involve moving some keys from CONFIG to the __init__ method of the class.
  • I also added the tests/test_coordinate_system.py tests. I chose the CoordinateSystem class to be tested because its CONFIG is being changed by this PR in a way that interacts with inheritance in non-trivial ways. The tests pass so I'm happy.

Future work

I believe this will be fine in terms of moving to attrs. Most of the attributes now defined in __init__ can easily be defined in __post__init__ or whatever it is.

NOTE: whoever merges this, please make sure to squash and delete all of the "merge X into Y" messages from the final commit....

leotrs added 20 commits August 27, 2020 23:53
…package by adding a config/__init__.py file. This will make it easier to continue refactoring the config system later
… class definition. Also add tests for this fact in tests/test_coordinate_system.py.
…so, fix mobject which was using the deprecated config[VIDEO_DIR]
@leotrs leotrs self-assigned this Sep 1, 2020
@leotrs leotrs added enhancement Additions and improvements in general refactor Refactor or redesign of existing code labels Sep 1, 2020
@leotrs
Copy link
Contributor Author

leotrs commented Sep 1, 2020

@huguesdevimeux @Aathish04 the tests/test_coordinate_system.py contains an example of how to use tempconfig to change the config for tests. What do you think about this? This does not yet mean we can choose a config file from the test suite, but it's getting closer. I think the next PR in this series (or the one after it...) will allow us to do so.

@Aathish04
Copy link
Member

the tests/test_coordinate_system.py contains an example of how to use tempconfig to change the config for tests. What do you think about this? This does not yet mean we can choose a config file from the test suite, but it's getting closer. I think the next PR in this series (or the one after it...) will allow us to do so.

I think this'll clear up a lot of confusion regarding exactly what config is used for which test. Eagerly awaiting the PR!

Copy link
Member

@huguesdevimeux huguesdevimeux left a comment

Choose a reason for hiding this comment

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

Very good. One step more against CONFIG !

@leotrs leotrs merged commit 4811562 into ManimCommunity:master Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general refactor Refactor or redesign of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants