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

Add defaults and validators for all valid config keys #488

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
2 participants
@jeremycline
Member

jeremycline commented Oct 26, 2017

This adds a lazily-loaded dictionary containing all valid fedmsg
configuration keys along with validators for all those keys to ensure
they are set to an acceptable value.

This new configuration dictionary is meant to replace the old
load_config API. That API has been marked as deprecated and will be
removed in a future release.

Signed-off-by: Jeremy Cline jeremy@jcline.org

@bowlofeggs

This comment has been minimized.

Member

bowlofeggs commented Oct 31, 2017

It looks like this is failing due to the new version of flake8 that checks for bare except statements. We had this problem with Bodhi too. You can either add an ignore on E722 to your flake8 config, or you can s/except:/except Exception:/ in the codebase. Looks like there aren't very many, so I'd recommend the latter of the two, and I'd recommend doing it in a separate commit or PR.

@bowlofeggs

This is a fine way to do it (and indeed is how Bodhi does it), but I recently learned that Colander is more generic than I realized and can do stuff like this:

https://docs.pylonsproject.org/projects/colander/en/latest/

Not at all saying you should switch since you already have this working, just thought you might find that library interesting to file away in your soft, human brain.

def _validate_non_negative_float(value):
"""
Assert a value is a non-negative integer.

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 31, 2017

Member

s/integer/float/

float: A non-negative floating point number.
Raises:
ValueError: if the value can't be cast to an integer or is less than 0.

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 31, 2017

Member

s/integer/float/

def _validate_none_or_type(t):
"""
Validate a setting is either None or a given type.

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 31, 2017

Member

You might want to express that this actually generates a validator that does this, since it itself does not do this.

Check the setting to make sure it's the right type.
Args:
object: The setting to check.

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 31, 2017

Member

s/object/setting/

return value
class FedmsgConfig(dict):

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 31, 2017

Member

Might want a docblock here.

#: The fedmsg configuration dictionary. All valid configuration keys are
#: guaranteed to be in the dictionary and to have a valid value. This dictionary
#: should not be mutated. This is meant to replace the old :func:`load_config`
#: API, but is not backwards-compatible with it.

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 31, 2017

Member

You could just use this text as the docblock for the class instead.

Add defaults and validators for all valid config keys
This adds a lazily-loaded dictionary containing all valid fedmsg
configuration keys along with validators for all those keys to ensure
they are set to an acceptable value.

This new configuration dictionary is meant to replace the old
load_config API. That API has been marked as deprecated and will be
removed in a future release.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the jeremycline:conf branch from 34b05e6 to 988dc5c Nov 2, 2017

@codecov

This comment has been minimized.

codecov bot commented Nov 2, 2017

Codecov Report

Merging #488 into develop will increase coverage by 1.78%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #488      +/-   ##
===========================================
+ Coverage    59.16%   60.95%   +1.78%     
===========================================
  Files           30       30              
  Lines         1854     1939      +85     
  Branches       302      325      +23     
===========================================
+ Hits          1097     1182      +85     
  Misses         670      670              
  Partials        87       87
Impacted Files Coverage Δ
fedmsg/commands/config.py 84.15% <0%> (+11.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0547bb...988dc5c. Read the comment docs.

@jeremycline jeremycline merged commit 0211e43 into fedora-infra:develop Nov 2, 2017

3 checks passed

codecov/patch Coverage not affected when comparing a0547bb...988dc5c
Details
codecov/project 60.95% (+1.78%) compared to a0547bb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremycline jeremycline deleted the jeremycline:conf branch Nov 2, 2017

@jeremycline

This comment has been minimized.

Member

jeremycline commented Nov 2, 2017

Colander has been filed away in my soft human brain for future investigation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment