Closed chshersh closed 1 year ago
Hi. So I have a looked a bit on this and I have a question.
"Extract cmdParserInfo from Iris.Env to Iris.Cli"
does this mean to move cmdParserInfo
into a file Iris/Cli/ParserInfo.hs
within the module Iris.Cli.ParserInfo
?
And pass it as an argument to mkCliEnv
?
@CThuleHansen You understand the required changes correctly with only a single change 😌
cmdParserInfo
as an argument to mkCliEnv
, let's make cmdParserInfo
accept CliEnvSettings
. And then we can pass all the settings from mkCliEnv
to cmdParserInfo
This way, in tests, we can construct ParserInfo
using cmdParserInfo
by specifying our test settings.
As I write this, I realise that it'll require to move CliEnvSettings
into their own module Iris.Settings
to avoid cyclic dependencies. But I'm fine with this, let's do this 😌
Okay, so I am approaching the target at https://github.com/CThuleHansen/iris and here is a small status and a few questions.
I have extracted and moved some bits according to our discussion, but I also moved cmd to Settings.hs, which seems off. But I need to access it from ParserInfo
, so it is to avoid cyclic dependencies.
Question: Is a Types.hs
file needed?
When I run my test, then --no-input
parses successfully. However, --help
gives a parserFailure
.
I am thinking this is because --help
is baked in and not added manually, whereas --no-input
is not.
The output for --help
seems correct besides the ParserFailure
:
Question: What to pass for ParserPrefs
in execParserPure
? These are not very well documented in https://hackage.haskell.org/package/optparse-applicative-0.17.0.0/docs/Options-Applicative-Extra.html#t:ParserPrefs
I have used Opt.ParserPrefs "suffix" False False False OptTypes.Backtrack 80 False False 0
What is prefMultiSuffix
? For the others, I just took default.
I am not getting the option for version
and numeric-version
because I have not passed any version settings yet.
This might fix the issue with help
as well. So I do not have any questions for this as of yet :)
I have extracted and moved some bits according to our discussion, but I also moved cmd to Settings.hs, which seems off. But I need to access it from ParserInfo, so it is to avoid cyclic dependencies. Question: Is a Types.hs file needed?
I see the dilemma. Putting Cmd
inside the Settings
modules feels off to me as well. In that case, let's put it into a separate module Iris.Cli.Cmd
. It's a part of CLI so it makes sense to keep it under a separate module. The data type is small at the moment but it'll get bigger. And putting it into a separate module will enable to write more detailed documentation on it 😌
I'm generally against generic modules like Types.hs
, Helper.hs
, Utils.hs
, Common.hs
and etc. So I'd like to avoid such modules in my projects. They tend to become a kitchen-sink of everything. It's worth thinking about a more appropriate place to put generic stuff in.
However, --help gives a parserFailure. I am thinking this is because --help is baked in and not added manually, whereas --no-input is not. The output for --help seems correct besides the ParserFailure
Indeed, this is the optparse-applicative
-specific behaviour. The --help
option is implemented ParserFailure
to stop parsing immediately and output the result of --help
. In that case, in tests for the --help
option, we should expect ParserFailure
and match the expected text with the hardcoded text of --help
.
Question: What to pass for ParserPrefs in execParserPure?
The Iris.Env
module uses the execParser
function to parse the CLI arguments. I see from its code that it uses the defaultPrefs
value. So I propose to use it in the tests as well.
Pull-request created: https://github.com/chshersh/iris/pull/68
I'm generally against generic modules like Types.hs, Helper.hs, Utils.hs, Common.hs and etc. So I'd like to avoid such modules in my projects. They tend to become a kitchen-sink of everything. It's worth thinking about a more appropriate place to put generic stuff in.
Yes, they certainly do.
The Iris.Env module uses the execParser function to parse the CLI arguments. I see from its code that it uses the defaultPrefs value. So I propose to use it in the tests as well.
I should have found this, thanks :)
It would be nice to test that the
--help
option by default outputs what we want. And it's also nice to see what changes when we change default parsers.So I propose to add a test that parses the
--help
option and compares it with the expected output of this option.The plan is:
Test.Iris.Cli
where the new test will livecmdParserInfo
fromIris.Env
toIris.Cli
--help
option using execParserPure fromoptparse-applicative