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

Convert ConfigParser object to dict and prevent #287 #402

Open
wants to merge 11 commits into
base: azren/read_only_config
Choose a base branch
from

Conversation

Azrenbeth
Copy link
Contributor

@Azrenbeth Azrenbeth commented Sep 16, 2021

Builds on #401. Followed by #404

Convert ConfigParser object to dict and prevent #287. Things no longer go into [DEFAULT] when the config file is generated (this is applying the fix mentioned in #314). Note that since the user's file is read after the CONFIG_DEFAULTS dict is looked at, placing them in [DEFAULT] or not has no affect on the running of the code, it only affects the generated config file.

SydentConfigParser is needed as previously only 2 of the config
options did not get interpolated (i.e. people could have been running
sydent with a config who's options referenced other options)

Using a dict means that knowledge of Python's configparser library's
get() method is no longer needed (which has a second argument which
is not the fallback).

Azrenbeth added 8 commits September 20, 2021 16:00
    SydentConfigParser is needed as previously only 2 of the config
    options did not get interpolated (i.e. people could have been running
    sydent with a config who's options referenced other options)

    Using a dict means that knowledge of Python's configparser library's
    get() method is no longer needed (which has a second argument which
    is not the fallback).

    The next step is to make having an option set to an empty string
    the same as not setting it.
If the config has:
option1 =

then get(option1) or None would be None rather than "". So to preserve
the meaning of "fallback" from ConfigParser it should be get(option1, None)
@Azrenbeth Azrenbeth changed the base branch from main to azren/read_only_config September 20, 2021 15:10
@Azrenbeth Azrenbeth marked this pull request as ready for review September 20, 2021 15:13
@DMRobertson DMRobertson requested a review from a team February 1, 2022 19:40
@babolivier
Copy link
Contributor

Note that since @Azrenbeth has finished his internship with us it's likely someone else will have to take this over.

@clokep clokep removed the request for review from a team February 3, 2022 13:40
@clokep
Copy link
Member

clokep commented Feb 3, 2022

Removing review since this depends on #401. If we want to merge this we'll need to dust that off first.

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.

3 participants