Change parsers to use flag' instead of switch
#4067
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markus1189
force-pushed
the
3959-flag-order
branch
from
98d25ce to
29a87d2
Compare
snoyberg
requested changes
Jun 6, 2018
| @@ -0,0 +1,2 @@ | |||
| main :: IO () | |||
| main = putStrLn "Test suite not yet implemented" | |||
There was a problem hiding this comment.
Can you make this a failing line so we're sure that the test suite isn't run?
| @@ -0,0 +1,65 @@ | |||
| # This file was automatically generated by 'stack init' | |||
There was a problem hiding this comment.
Can you replace this with resolver: lts-11.6
| @@ -0,0 +1,6 @@ | |||
| module Lib | |||
There was a problem hiding this comment.
It should work to drop this module.
| @@ -0,0 +1,61 @@ | |||
| -- This file has been generated from package.yaml by hpack version 0.28.2. | |||
There was a problem hiding this comment.
I'd recommend replacing this .cabal file with a package.yaml file instead.
| @@ -0,0 +1,6 @@ | |||
| module Main where | |||
| @@ -0,0 +1,2 @@ | |||
| import Distribution.Simple | |||
snoyberg
reviewed
Jun 6, 2018
| @@ -19,7 +19,7 @@ benchOptsParser hide0 = BenchmarkOptsMonoid | |||
| help ("Forward BENCH_ARGS to the benchmark suite. " <> | |||
| "Supports templates from `cabal bench`") <> | |||
| hide)) | |||
| <*> optionalFirst (switch (long "no-run-benchmarks" <> | |||
| <*> optionalFirst (flag' True (long "no-run-benchmarks" <> | |||
There was a problem hiding this comment.
Can you add a comment to the changelog about the bugfix?
markus1189
force-pushed
the
3959-flag-order
branch
from
29a87d2 to
7d50f47
Compare
This fixes commercialhaskell#3959. The problem is that the [switch](https://hackage.haskell.org/package/optparse-applicative/docs/Options-Applicative-Builder.html#v:switch) function returns a `Just False` which gets wrapped into `First`. This means that the value is *always* set to either `True` or `False` and the `First` monoid means that during options parsing the order of the flags suddenly matters as described in commercialhaskell#3959, because `First (Just False)` is chosen when `mappend`ing with `First (Just True)`. The fix is to use the [flag](https://hackage.haskell.org/package/optparse-applicative-0.14.2.0/docs/Options-Applicative-Builder.html#v:flag-39-) function that does not set a default value, so we get a `First Nothing` instead.
markus1189
force-pushed
the
3959-flag-order
branch
from
7d50f47 to
bf4711e
Compare
mihaimaruseac
approved these changes
Jun 9, 2018
snoyberg
approved these changes
Jun 12, 2018
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #3959.
The problem is that the switch function returns a
Just Falsewhich gets wrapped intoFirst.This means that the value is always set to either
TrueorFalseand the
Firstmonoid means that during options parsing the order ofthe flags suddenly matters as described in #3959, because
First (Just False)is chosen whenmappending withFirst (Just True).The fix is to use the flag function that does not set a default value, so we get a
First Nothinginstead.Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.
Please include the following checklist in your PR:
there should be no user relevant changes
no relevant changes
Please also shortly describe how you tested your change. Bonus points for added tests!
There is a new integration test in the
3959-order-of-flagsfolder that makes sure that the order does not matter.