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

Optionally disable 'environment' feature #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Optionally disable 'environment' feature #4

wants to merge 2 commits into from

Conversation

timvaillancourt
Copy link

This is a working proof-of-concept to disable the environment feature - something I want to do in one of my use cases of yconf.

I am open to any changes/feedback to improve this change, match the code/naming style better, etc.

Also: seeing a 2nd bool arg is being added to BaseConfiguration.init, maybe this should move to keyword-args, like this below? I did that in my first test but didn't want to get carried away :)

class BaseConfiguration(NestedDict):

    _environments = ("production", "staging", "development")

    def __init__(self, **args):

        NestedDict.__init__(self, {})

        try:
            self.merge = args['merge']
        except:
            self.merge = True
        try:
            self.do_environment = args['do_environment']
        except:
            self.do_environment = True

@kampka
Copy link
Owner

kampka commented Oct 13, 2016

First, all pull requests should come with new tests that verify the change works as expected and ideally also demonstrates the intended use case.

Second, all pull requests should pass the tests ;)

Can you explain to me what your use case is here?
If you only want to get rid of the environment root in the yaml tree, I am not convinced this use case is worth supporting. You can also do this already by subclassing the BaseConfiguration and overriding load_config() to do that (granted, that would leave you with an unsued -e argument on the parser).

@timvaillancourt
Copy link
Author

Thanks, ok.

Interesting, yes I would optionally disable the 'concept' of environments in one use case because that app just never had any idea of config-environments before and it already uses -e for a different option.

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