Skip to content

Conversation

@leotrs
Copy link
Contributor

@leotrs leotrs commented Aug 30, 2020

Context

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

Main change

The main goal here is to encapsulate everything having to do with setting up the config in a new folder config/. Before, setting up the config and parsing the .cfg files was strewn about in many files. Now... it is still fairly disordered and in many files, but at least they live in the same directory. The next PR in this series will start merging those files, now within the safety of the new subpackage.

Yes, there are 30 files touched by this PR but most of them are one-line modifications of import statements. The actual PR is like two files and a bunch of renamed/moved files.

The motivation

The main benefit of this change is that after the config is set up, no other part of the codebase needs to care about how it is being setup. Accordingly, many imports of the form from ..config import camera_config or similar have been changed to from .. import camera_config. The former says "go to the config module and get me the configuration of the camera", and it required of the developer that they know all of the internals of how the config system is set up. For example, if the configuration system changed (say is camera_config was defined in some other module), all of those imports had to change. The new version from .. import camera_config says "go to the top level package and get me the configuration of the camera". Even if the configuration system changes internally, as long as the config subpackage exports camera_config to the top-level package, this import will continue to work always.

Additionally, the logger.py module is also now part of the config/ folder, as it deals with parsing .cfg files and needs to export a variable that all other modules need as soon as possible (the logger).

List of changes

  1. init.py now just imports * from the config subpackage.
  2. There is a new folder config/ which is made into a subpackage by adding the file config/__init__.py. Remember, a subpackage is nothing more than a collection of modules and the config/__init__.py file just determines what is exported into the top level package. The new subpackage contains all of the modules and resources related to setting up the config system.
  3. A bunch of imports across the codebase have been simplified.
  4. Added a couple of tests to check that everything is being exported adequately.

Future work

Encapsulating everything inside a subpackage will make it super easy in the future to keep refactoring the config system with minimal modification of the rest of the codebase. See #337 for future plans on refactoring the config system.

EDIT: sorry for the messy commit history. I'm... trying something.

@leotrs leotrs added pr:deprecation Deprecation, or removal of deprecated code refactor Refactor or redesign of existing code pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! labels Aug 30, 2020
@leotrs leotrs self-assigned this Aug 30, 2020
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.

LGTM, nice cleanup.

The doctest fails seem weird to me, locally all tests pass for this branch.

@leotrs
Copy link
Contributor Author

leotrs commented Aug 31, 2020

Ok I think I know what's wrong with the tests. This PR moves the file default.cfg and so it is no longer being shipped with the rest of the package so when GH actions installs manim using pip, it can't find it. I can fix the setup.py in a sec.

@naveen521kk can we make sure this doesn't happen with poetry? The default.cfg file must always be shipped/packaged.

@naveen521kk
Copy link
Member

can we make sure this doesn't happen with poetry? The default.cfg file must always be shipped/packaged.

Yes, I had included everything in manim folder inside package.

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

Labels

pr:deprecation Deprecation, or removal of deprecated code pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! refactor Refactor or redesign of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants