-
Notifications
You must be signed in to change notification settings - Fork 116
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
Allow options via environment variables #118 #435
base: master
Are you sure you want to change the base?
Conversation
Thanks for contributing and interesting work. I won't comment now about the PR code – I will have a look at the implementation in more depth in a coming weekend. Even though this is not a lot of code, it's a big change which broadens the scope from command line parsing to general configuration. There are things to think about, such as:
Lots to consider. Kind regards, |
Thank you for your response and awesome library btw. I agree that there are "Lots to consider". That's why I wanted to get your opinion on the approach before diving into details.
There can be few options:
I should note, that I'm still not so much familiar with internals of the library, so it is quite possible that there can be more elegant way. Why modifier? In most cases, environment variable is "linked" to some cmd option, so it looks reasonable to make it option modifier rather then separate top-level parser. Regarding Adding
I believe
I don't see how they can interact. I mentioned in PR comment we likely need similar modifier for
Good question. I personally felt lack of this feature many times. Many tools (systemd, cron, docker and modern deployment infrastructure at all etc) are built around environment as source of customization values. On the other hand for local development and use cmd options are often more convenient. So you end up with boilerplate wrapper scripts like:
Beside for being tedious and error-prone you cannot list possible environment variables in help text (and documentation you could generate from parser description) without manually adding them to Thus I would say there are 2 key benefits:
How big is this gain? For me (and other people voted for initial request) it is worth changes, but it is up to you to decide keeping in mind all those things important for such popular library like compatibility, maintainability, lightweightness and so on. And I know you are very reasonably keen on this. Another question that has been raised and should be kept in mind: is this in scope of this library at all? I agree with this comment. As environment and cmd options often closely interact and complement each other it is reasonable to support both. Ofc if to view |
Hello, I'd be interested in completing the implementation, how can I help? |
@thejohncrafter thank you for raising this! I think there are still some open questions and the first one is: do we want these changes at all? The question is quite debatable, so feel free to share your thoughts on this. I would like to come to the agreement on this point before addressing other questions mentioned in the discussion. @HuwCampbell what do you think? |
@stevladimir thank you for starting this PR! I think the ability to read environment variables is relevant for this library. To be honest, I need this for a project of mine, and it looks like I can't do it properly without modifying The whole other question is that of the wording. I haven't been in the details of the library for now, so I can't tell what is the best way to implement this. |
I think so too, but it is important that maintainers of this library also support this point of view.
Basically, you can use this PR if you only need an ability to pass options via environment. It is missing help texts and so on, cause there is still some uncertainty about how to handle this better. |
I'm on holiday at the moment. Back around the 15th. In general I'm weighing compatibility, utility over known work arounds, and the api. I'm kind of hopeful there's a neat solution. We should look hard at decline, clap, and nest before getting too far into implementation. |
Let's figure out a list of things to reason about and go over it. I will start:
|
Sketched up the solution.
It implies changes to core parser type, but tried to keep changes to API as minimal as possible. Atm it lacks necessary changes to parser docs generation and maybe some other things.
There are also few open questions:
Widely accepted convention is cmd options taking precedence over environment variables, however, in issue discussion this question was raised. So need to decide whether it should be customizable.
$FOO
when usingenviron "FOO"
. In that case we also minimize changes to help text as it becomes self-documenting.environ
.Smth like
[ -n "${FOO:+x}" ]
is often used and works as a boolean flag "is variable is set and not null". But this is not the only way, so we should make this customizable.One of the solutions is to change
environ
type toReadM a -> String -> Mod f a
. This will also make possible to use different readers for environment and cmd parser, but I personally don't like this.We can also have separate combinators
envOpt :: String -> Mod OptionFields a
andenvFlag :: String -> ReadM a -> Mod FlagFields a
. Or just addenvReader :: ReadM a -> Mod FlagFields a
and have some default reader for flags.