Skip to content

default.cfg isn't documented #178

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

Closed
XorUnison opened this issue Jul 7, 2020 · 8 comments
Closed

default.cfg isn't documented #178

XorUnison opened this issue Jul 7, 2020 · 8 comments
Labels
documentation Improvements or additions to documentation

Comments

@XorUnison
Copy link
Collaborator

Now that #98 has been merged it has replaced the temporary dirs.py I added exactly like I had envisioned that happening. So now it's time to open this issue on one last improvement on that front I'd like to see.

See, both dirs.py and default.cfg had/have this one issue, that's most relevant to devs, but not irrelevant to standard users either. When default.cfg is edited, Git will recognize this as a change that might be uploaded to the repo, which of course isn't ever the case. While this can easily be ignored locally, switching branches will overwrite the default.cfg. Further if we ever change the default.cfg, which we should do as rarely as possible but let's be real, it will probably happen a few times, this also would overwrite any custom changes by the user, and this last point is also what's affecting all users, not just devs.

The question is, what do we actually do about that? I've thought about it for a while but I'm not entirely sure what's the best approach. The best idea I had involves versioning and is this:

  1. Rename default.cfg to [version].cfg and add internal cfg versioning
  2. Add default.cfg to .gitignore (which btw still contains a reference to media_dir.txt we can delete with the same PR)
  3. References still look for a default.cfg, and if it's not found will offer to rename [version].cfg into default.cfg, making sure that hassle for end users is kept to an absolute minimum
  4. At this stage the user only needs to get a default.cfg once which can be automatically done on the CLI, after which that default.cfg interferes neither with dev work like branch switching, nor will it ever be overwritten by updates. Necessary updates instead include a [newer version].cfg and will throw a warning/error at users, informing them of the need to migrate their changes to the newer version.

Thoughts are any issues with this plan?

@XorUnison XorUnison added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Jul 7, 2020
@leotrs
Copy link
Contributor

leotrs commented Jul 7, 2020

Apologies for my lack of communication about this. But the current config system already solves this, as follows. As you correctly point out, the default.cfg should rarely, if ever, be changed. This is meant to be a library-wide default config. No one should change the default config, unless the dev team decides that a new/better default is needed.

When a user wants to change the config, there are other files they need to create/modify. The current config system already supports this. The idea is to have a "cascading" set of files, each overriding the previous one. See this line in config.py. So in order to change the default library-wide configuration, a user can create a new .manim.cfg file in their home directory. For example, if they want to keep using all library-wide defaults and change ONLY the background color of their scenes, they need to create this file:

[CLI]
background_color = some_color_here

and save it as .manim.cfg.* This file is always going to be read, if it exists, and any config options set in it will always override the library-wide defaults.

Lastly, if the user wants to change the config on a per-run basis, the current config system also supports the --config_file flag. As you can see here, this file overrides BOTH the user-wide and the library-wide configs.

Finally, if there is a manim.cfg file in the same directory as the scene file AND the --config_file flag is not sued, that file is also used to override the user-wide and the library-wide configs, see here.

I'm open to discussion of the locations, names, and priorities of these cascading config files, but I think the current system works and fixes the problem you are bringing up. What do you think?


  • This of course works for linux. We should discuss what the user-wide config file name should be for Windows.

@kilacoda
Copy link
Contributor

kilacoda commented Jul 7, 2020

Why not just add it to the .gitignore?

@leotrs
Copy link
Contributor

leotrs commented Jul 7, 2020

I think I beat @raghavg123 to the punch ;)

But to be completely clear: the whole point of the config system use .cfg files is that users who want to customize how manim works do not need to write python code and do not need to delve into the depths of the library itself. Asking users to change a file that is within the library goes against that philosophy. In some systems, it is not even obvious where the default.cfg file would be found.

@XorUnison
Copy link
Collaborator Author

XorUnison commented Jul 7, 2020

Apologies for my lack of communication about this.

Right, I guess this is the real issue then.

What do you think?

Looks perfectly fine and better than my solution. This already works just fine on windows though so I see no need for changes there?

Shifting gears to communication then, obviously end users aren't going to figure this out themselves. As easy as it is to use, it isn't intuitive at all. Is that in some documentation already? Also tbh a small hint in the default.cfg file would be prudent.

@XorUnison XorUnison added documentation Improvements or additions to documentation and removed pr:bugfix Bug fix for use in PRs solving a specific issue:bug labels Jul 7, 2020
@leotrs
Copy link
Contributor

leotrs commented Jul 7, 2020

My next step in terms of the config system is definitely a more extensive documentation. I like your idea of also leaving a comment inside default.cfg. Some thing like DO NOT EDIT THIS FILE. USE <such and such> FILE INSTEAD.

This already works just fine on windows though

That's what I thought, I just hadn't tested it yet.

@eulertour eulertour changed the title default.cfg and the issue of updating default.cfg isn't documented Jul 7, 2020
@eulertour
Copy link
Member

Speaking of this, is there anything blocking us from generating documentation now? @PgBiel

@leotrs
Copy link
Contributor

leotrs commented Jul 8, 2020

So, starting out with documentation is as easy as: start writing docstrings more systematically (though #174 should be decided upon before that). And then running sphinx with default settings. Changing the sphinx settings in the future should not affect the actual docstrings (or other documentation files).

I know @PgBiel wants to take the lead on this, but I also know they're in exams right now :)

@leotrs
Copy link
Contributor

leotrs commented Aug 26, 2020

I'm closing this as there now exists a file documenting the config system in docs/build/tutorials/configuration.html. This file is far from complete, but at least it explains the cascading system, which is the subject of this issue.

@leotrs leotrs closed this as completed Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants