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

make the prefix for environment variables alterable #931

Merged
merged 4 commits into from
Sep 5, 2017

Conversation

Archelyst
Copy link
Contributor

Suppose my app is named "Abc", it is really not obvious, why the configuration prefix for environment variables is "SANIC_". Also, if I happen to have more than one Sanic app in one environment, there would be no way to distinguish those. Hence, I propose to make it possible to use other prefixes.

@jrocketfingers
Copy link
Contributor

LGTM, except that it'd be cool if we documented it too.

@jrocketfingers
Copy link
Contributor

@Archelyst
Copy link
Contributor Author

Archelyst commented Sep 5, 2017

Great, now I get to be the one to write the docs for something already existing *grumble*

Well, you're right, I'll add some docs :)

@Archelyst
Copy link
Contributor Author

Oh, I just saw, there is already documentation for that, my fault. Of course I'll update it.

@Archelyst
Copy link
Contributor Author

I just realized, that - while this change is sufficient for my use case - many people will probably use the load_env option. What do you think about allowing to pass in the prefix via this load_env variable? E.g.

app = Sanic(load_env='MYAPP_')

@Archelyst
Copy link
Contributor Author

I added the possibility to specify the prefix through the app constructor, added docs and tests.

Also I fixed the existing tests, two of the functions had the same name which caused one of them (the failing one) to be skipped.

@r0fls r0fls merged commit 8b4ca51 into sanic-org:master Sep 5, 2017
@seemethere seemethere added this to the 0.6.1 milestone Oct 13, 2017
@seemethere seemethere modified the milestones: 0.6.1, 0.7.0 Nov 13, 2017
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.

4 participants