Skip to content
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

Moved config.ini.example to config.defaults.ini #109

Closed
wants to merge 1 commit into from

Conversation

sedrubal
Copy link
Member

I think later we should change the run.py parameters. I added a TODO in the code:

# TODO restructure this: simply provide a --config-file parameter but
# always load config.defaults.ini first and the the users config

@landscape-bot
Copy link

Code Health
Repository health increased by 0.06% when pulling fafec04 on improve-config-system into 4a4d884 on development.

@mgmax
Copy link
Member

mgmax commented Nov 22, 2015

Renaming the config without changing the way the config system works is confusing, because the new name is just wrong. This should not be merged yet.

@sedrubal sedrubal force-pushed the improve-config-system branch from fafec04 to 8a5a637 Compare November 22, 2015 14:33
@landscape-bot
Copy link

Code Health
Repository health increased by 0.06% when pulling 8a5a637 on improve-config-system into ce0d8b3 on development.

@sedrubal
Copy link
Member Author

I changed the system by loading config.defaults.ini before loading config.ini, but I forgot to mention this in the commit message. See 8a5a637#diff-22cd404ff64283f596d2fe30d447f59dR96

(I just force pushed, because the PR was not clean. It contained changes of another PR)

@@ -4,7 +4,7 @@ TODO
Needed testing for FAU FabLab
-----------------------------

cp config.ini.example config.ini
cp config.defaults.ini config.ini
Copy link
Member

Choose a reason for hiding this comment

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

well, this is not really true anymore...

@sedrubal
Copy link
Member Author

This PR does not completely fix the broken config system, but it might be a step. The milestone ImproveConfig might help to keep in mind that further changes are required.

@patkan
Copy link
Member

patkan commented Nov 23, 2015

But doesn't this code simply load config.defaults.ini and then discard these settings and load config.ini if the latter file is present?
Or do these settings "overload" the old settings (thus adding only the changed parameters).

@patkan patkan assigned sedrubal and unassigned patkan Nov 23, 2015
@sedrubal
Copy link
Member Author

As far as I read and tested, the second reading overloads the settings of the first reading.

Example:

  • config.defaults.ini contains support_mail but config.ini doesn't, the value set in defaults will be taken
  • config.defaults.ini contains support_mail = change@me and config.ini contains support_mail = [email protected] the latter one will be accessible

@sedrubal
Copy link
Member Author

Oh, I think we want to do it like this example: https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser.read (but maybe we want to check if config.ini exists)
#rtfm

@mgmax mgmax added the wip label Nov 27, 2015
@mgmax
Copy link
Member

mgmax commented Nov 27, 2015

From the source it looks quite good, but the documentation and handling of creating the first config needs to be changed. We don't really need a config anymore, maybe FabLabKasse should create some empty dummy config file with short instructions?

@mgmax
Copy link
Member

mgmax commented Apr 22, 2023

closing as WONTFIX

@mgmax mgmax closed this Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash on missing config option
4 participants