Skip to content

Commit

Permalink
Change parsers to use flag' instead of switch
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
markus1189 committed Jun 6, 2018
1 parent 8ba673f commit bf4711e
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 3 deletions.
2 changes: 2 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ Bug fixes:
was tried to be registered. This is now fixed by always building internal
libraries. See
[#3996](https://github.com/commercialhaskell/stack/issues/3996).
* Order of commandline arguments does not matter anymore.
See [#3959](https://github.com/commercialhaskell/stack/issues/3959)


## v1.7.1
Expand Down
2 changes: 1 addition & 1 deletion src/Stack/Options/BenchParser.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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" <>
help "Disable running of benchmarks. (Benchmarks will still be built.)" <>
hide))
where hide = hideMods hide0
4 changes: 2 additions & 2 deletions src/Stack/Options/TestParser.hs
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ testOptsParser hide0 =
help "Arguments passed in to the test suite program" <>
hide)))
<*> optionalFirst
(switch
(flag' True
(long "coverage" <>
help "Generate a code coverage report" <>
hide))
<*> optionalFirst
(switch
(flag' True
(long "no-run-tests" <>
help "Disable running of tests. (Tests will still be built.)" <>
hide))
Expand Down
21 changes: 21 additions & 0 deletions test/integration/tests/3959-order-of-flags/Main.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import StackTest

import Control.Monad (unless)
import Data.List (isInfixOf)

-- Integration test for https://github.com/commercialhaskell/stack/issues/3959
main :: IO ()
main = do
checkFlagsBeforeCommand
checkFlagsAfterCommand

checkFlagsBeforeCommand :: IO ()
checkFlagsBeforeCommand = stackCheckStderr ["--test", "--no-run-tests", "build"] checker

checkFlagsAfterCommand :: IO ()
checkFlagsAfterCommand = stackCheckStderr ["build", "--test", "--no-run-tests"] checker

checker :: String -> IO ()
checker output = do
let testsAreDisabled = any (\ln -> "Test running disabled by" `isInfixOf` ln) (lines output)
unless testsAreDisabled $ fail "Tests should not be run"
10 changes: 10 additions & 0 deletions test/integration/tests/3959-order-of-flags/files/package.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
name: issue3959
version: 0.1.0.0

dependencies:
- base

tests:
test:
main: Spec.hs
source-dirs: test
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
resolver: lts-11.6
2 changes: 2 additions & 0 deletions test/integration/tests/3959-order-of-flags/files/test/Spec.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
main :: IO ()
main = fail "this always fails for the test"

0 comments on commit bf4711e

Please sign in to comment.