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

Strategies #20

Closed
wants to merge 35 commits into from
Closed

Strategies #20

wants to merge 35 commits into from

Conversation

hernantz
Copy link
Contributor

@hernantz hernantz commented Dec 26, 2018

Here is my attempt at improving prettyconf's strategy for configuration discovery #18.
It introduces breaking changes in the API so a version bump will be needed if this gets out.

Probably the most controversial change is the Commandline loader. I had to do some black magic to check if a given config was set by the user or not by cloning the parser. The problem with cli parsers is that they also support setting defaults, which would interrupt the dicovery chain.

The other important change is that loaders will not glob (*.ini, *.cfg) any more. The user has to explicitly use EnvFile(filename='/path/to/.env') or IniFile(filename='/path/to/settings.ini). This is because a project might include many files like prod.env, staging.env or even things that are unrelated, like a pytest.ini file.

This PR is still WIP. I wanted you to take an early look on it.

  • Improve docs for loaders (add autodoc maybe?)
  • Refactor tests.
  • Example project that uses the same codebase for ansible/docker/etc.
  • Document the changes in CHANGES.txt

docs/source/loaders.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
@osantana
Copy link
Owner

osantana commented Jan 2, 2019

I really like the refactoring but I'd like to preserve the current behavior to avoid problems with backward compatibility.

The proposed behavior must be implemented as an optional configuration that must be explicitly defined.

prettyconf/loaders.py Show resolved Hide resolved
@hernantz
Copy link
Contributor Author

hernantz commented Jan 11, 2019

I've added some changes:

  • New RecursiveSearch loader that discovers .env and *.ini/*.cfg files, this is the new default along with Environment.
  • New strategy for the CommandLine loader, which consists on asking the user to set a special default value that will allow prettyconf to keep the lookup going, i.e:
    from prettyconf import Configuration, UNSET
    from prettyconf.loaders import CommandLine
    
    import argparse
    
    parser = argparse.ArgumentParser(description='Process some integers.')
    parser.add_argument('--debug', '-d', dest='debug', default=UNSET, help='set debug mode')
    
    c = Configuration(loaders=[CommandLine(parser=parser)])
    print(c('debug', default=False, cast=c.boolean))
  • Added autodoc for sphinx

Still need to go through the bullet points above, any comments are welcome.

@osantana
Copy link
Owner

I've added some changes:

Cool! I'll take a deeper look tonight (in Brazil) so we talk a little bit about the PR.

Copy link
Owner

@osantana osantana left a comment

Choose a reason for hiding this comment

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

It's getting really good!

prettyconf/configuration.py Show resolved Hide resolved
prettyconf/loaders.py Outdated Show resolved Hide resolved
prettyconf/loaders.py Outdated Show resolved Hide resolved
prettyconf/loaders.py Outdated Show resolved Hide resolved
prettyconf/loaders.py Show resolved Hide resolved
prettyconf/loaders.py Show resolved Hide resolved
prettyconf/loaders.py Show resolved Hide resolved
prettyconf/loaders.py Show resolved Hide resolved
prettyconf/loaders.py Show resolved Hide resolved
prettyconf/loaders.py Show resolved Hide resolved
prettyconf/loaders.py Outdated Show resolved Hide resolved
prettyconf/loaders.py Outdated Show resolved Hide resolved
@osantana
Copy link
Owner

Now, we'll have to "fight" against Travis to make it works :/

Copy link
Owner

@osantana osantana left a comment

Choose a reason for hiding this comment

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

Amazing job! Thanks for these contributions.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.2%) to 94.828% when pulling 5e9839b on hernantz:strategies into 3ec45b6 on osantana:master.

@coveralls
Copy link

coveralls commented Jan 29, 2019

Coverage Status

Coverage decreased (-5.2%) to 94.828% when pulling 5f3471a on hernantz:strategies into 3ec45b6 on osantana:master.

@osantana
Copy link
Owner

Let's remove support for Python 2 at CI. Keep support only for py36 and py35 for linux+mac. I want to keep support for pypy3, but it's optional.

I can help you with that task if you wish.

@hernantz
Copy link
Contributor Author

hernantz commented Jan 29, 2019 via email

@osantana
Copy link
Owner

Sure, help would be great!

I'll have some time tomorrow ~8pm BRST. When we fix the build I merge and prepare a major release.

@osantana
Copy link
Owner

I've fixed build and made some improvements on your PR and open a new PR #21. You decide if you want to cherry pick commits to your PR or move the development to #21 (I'll add you as a collaborator of the project to make your life easier)

@osantana osantana closed this Feb 1, 2019
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