-
Notifications
You must be signed in to change notification settings - Fork 147
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
Support mapping env vars to multiple settings #189
Conversation
1 similar comment
Hello @twitchard. I really do appreciate the way you have prepared the 2 competing PRs. Thanks! For now I have accepted #188. What do other interested parties think about this PR please? |
I'm convinced by @twitchard' rationale. And I'm for accepting this PR. I plan to merge it just after #191 lands. |
@twitchard could you rebase your commit and resolve the conflicts please? I would be very happy to then merge your PR. Thank you! |
03eb43a
to
deb0767
Compare
deb0767
to
29ed406
Compare
Done! |
2 similar comments
You're so fast guys 😄 Thanks @twitchard! |
Addresses issue #124
I didn't add an overridable option to toggle the old behavior and throw on envvar reuse. My argument is as follows:
convict(...)
would add too much complexity, because no such optional parameter already exists.If you're not persuaded, I would be happy to try a different approach along the lines of #2 in my list if you think that is best. Otherwise, I've also made a PR #188 that documents and adds a test for the current behavior.