Skip to content

config.py: merge default module configs into user config - #218

Merged
richrd merged 1 commit into
richrd:devfrom
Consolatis:feature/default_module_configs
Mar 7, 2018
Merged

config.py: merge default module configs into user config#218
richrd merged 1 commit into
richrd:devfrom
Consolatis:feature/default_module_configs

Conversation

@Consolatis

@Consolatis Consolatis commented Jan 4, 2018

Copy link
Copy Markdown
Contributor

This allows modules which provide a method called get_default_config() to return a dict of own default settings which will then be merged into the global config as config["modules"][$module_name].
This can then be modified by a user config using the same path.

The idea is that modules can provide their own default config without modifying defaults.json and still allow the user to modify that config which can be useful for e.g. out-of-tree modules.
It also prevents clutter of defaults.json with config options for modules which are not actually used by the user.

Another possible use case would be to have every module expose a default boolean config option like "enabled" which can default to false. Then the user can manually enable modules which are more of special case modules and does not have to delete or move the module file itself.

Instead of testing if the module provides a get_default_config() function it could also be implemented in the module base class.

Things to do:

  • add get_default_config() as stub inside module base class
  • rebase on dev
  • squash

@Consolatis
Consolatis force-pushed the feature/default_module_configs branch from e7f6398 to 14f153f Compare January 4, 2018 22:17
@richrd

richrd commented Jan 21, 2018

Copy link
Copy Markdown
Owner

Seems like a rather good idea to me! I would say that it'd make sense for the module base class to provide get_default_config. That way we avoid the special check in config.py.

@Consolatis
Consolatis force-pushed the feature/default_module_configs branch from 14f153f to c6006d2 Compare January 23, 2018 12:30
@Consolatis

Consolatis commented Jan 23, 2018

Copy link
Copy Markdown
Contributor Author

I would say that it'd make sense for the module base class to provide get_default_config

Added. If you are generally fine with the idea, anything else to change?

I am not sure if I am using "module_name" correctly, there is

  • a filename without .py extension (__name__)
  • the name inside the module dict inside a module file

@Consolatis Consolatis changed the title [Not to merge without discussion] config.py: merge default module configs into user config [RFC] config.py: merge default module configs into user config Jan 23, 2018
@Consolatis Consolatis mentioned this pull request Jan 26, 2018
11 tasks
@richrd

richrd commented Feb 26, 2018

Copy link
Copy Markdown
Owner

I am not sure if I am using "module_name" correctly

To be honest I'm not sure I designed that part correctly. I can't even remember how it works now. I'll have to check.

EDIT:
I checked. The name defined within the file is not even used anywhere. Sadly I can't remember why I added it in the first place. Anyway that said you are using it correctly. Basically the filename is the actual name that is used. This could be changed in the future just by changing the logic in module_loader.py. That way the names could be independent from the filename.

@richrd

richrd commented Feb 26, 2018

Copy link
Copy Markdown
Owner

I think this is ready for merging after rebase+squash. The idea that the defaults are specified in the module and not in the default configuration is perfect! Thanks 👍

@Consolatis
Consolatis force-pushed the feature/default_module_configs branch from c6006d2 to 5209aab Compare March 1, 2018 11:33
@Consolatis

Copy link
Copy Markdown
Contributor Author

done, rebased and squashed

@Consolatis Consolatis changed the title [RFC] config.py: merge default module configs into user config config.py: merge default module configs into user config Mar 1, 2018
@richrd

richrd commented Mar 7, 2018

Copy link
Copy Markdown
Owner

Great! Thanks :)

@richrd
richrd merged commit 3d7ede2 into richrd:dev Mar 7, 2018
@Consolatis
Consolatis deleted the feature/default_module_configs branch March 8, 2018 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants