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

Added namespaced environment convention advice #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hernantz
Copy link
Contributor

I believe documentation should also teach good practices. Added a notice for how to properly name an environment setting.
I thought about adding an option like Environment(prefix='MY_APP_') but we already have the var_format option so it should suffice for now.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.294% when pulling fecd8b9 on hernantz:improve-docs-for-namespaced-environment into 75c55a2 on osantana:master.

@osantana
Copy link
Owner

When I created prettyconf (and even today) I did it to use with Heroku PaaS. The database connection info provided by them is in DATABASE_URL env var. In the code example, we will break this use case.

I think we can recommend this practice for custom/user environment variables but not recommend the code that adds a prefix all variable names.

@hernantz
Copy link
Contributor Author

This PR does not involve any changes in the code. The idea here was to suggest good practices in the docs. You would still have the flexibility to follow or ignore them when it makes sense.

I've used prettyconf because it has an agnostic approach to configure applications, no matter if they are web, cli or gui apps, hosted on the cloud or running in your desktop.

In the case of CLI apps, it would be recommended to set some sort of namespace so that you don't accidentally override other programs behaviour, like LOCALE or EDITOR, but instead MY_APP_LOCALE, etc.

Yes, in case of a web app running in heroku it would not make sense, as it would not make sense to use any other loader like IniFile or RecursiveSearch for example.
Maybe we could add more context in the docs or change them so that this idea is clearer?

@osantana
Copy link
Owner

This PR does not involve any changes in the code. The idea here was to suggest good practices in the docs. You would still have the flexibility to follow or ignore them when it makes sense.

Yeah. Now it's clear to me. And I agree with you.

Maybe we could add more context in the docs or change them so that this idea is clearer?

Sure. Maybe it'd be a good idea to explain the reason why it's a good practice would improve the documentation.

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