Skip to content

Conversation

@knight9999
Copy link
Contributor

Description of the change

This PR is to fix the bug that the user can not spago run command when he specifies the output directory different from output.

  • The Issue I found

Support user wants to specify output directory, for example, output2 directory.

The build can be done by the command

$ spago build -u "-o output2"

However, the run leads to error by the command.

$ spago run -u "-o output2"
  • Reason and how to fix in this PR

This issue is because findFlag function in Path.hs has a problem.
If we use

$ spago run -u "-o output2"

, the pursArgs is ["-o output2"], not ["-o", "output2"].

Therefore (x:y:xs) can not match the y = "output2".

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

@f-f
Copy link
Member

f-f commented Sep 20, 2020

Hi @knight9999, thanks for contributing the fix! 👏

This was sort of a known bug for a while, but not enough people stumbled on the edge cases for it to be annoying. In fact you can see I left a "TODO add tests" just a few lines above your fix:

-- TODO tests:
-- ["-o", "something"]
-- ["--output", "something"]
-- ["--output something"]
-- [" -o something"]
-- ["--output=something"]

It would be great if you could add these tests as part of this PR, as it might help catch any other bugs we might be introducing (for example, there seems to be a regression in the current tests)

@knight9999 knight9999 changed the title fix: to correct value of pursArgs in findFalg in Path.hs fix: to correct value of pursArgs in findFlag in Path.hs Sep 20, 2020
@knight9999
Copy link
Contributor Author

knight9999 commented Sep 20, 2020

Thanks @f-f for the valuable advice.
Now I have just added the test case and improved my PR to support --output=something case.

There are some issues in test cases that are not related with my PR,
Please test by using Spec.hs as

module Spec (spec) where

import Test.Hspec

import qualified Spago.Command.PathSpec as PathSpec

spec :: Spec
spec = do
  describe "PathSpec"     PathSpec.spec

instead of the original one

{-# OPTIONS_GHC -F -pgmF hspec-discover -optF --module-name=Spec #-}

temporaly

@knight9999
Copy link
Contributor Author

Thanks @f-f for the valuable advice.
Now I have just added the test case and improved my PR to support --output=something case.

There are some test issue other than my PR,
Please test by using Spec.hs as

module Spec (spec) where

import Test.Hspec

import qualified Spago.Command.PathSpec as PathSpec

spec :: Spec
spec = do
  describe "PathSpec"     PathSpec.spec

instead of the origin one

{-# OPTIONS_GHC -F -pgmF hspec-discover -optF --module-name=Spec #-}

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a small comment but otherwise this looks great 👏

Improve comments aboute tests

Co-authored-by: Fabrizio Ferrai <[email protected]>
@knight9999
Copy link
Contributor Author

Hi @f-f . Thank you very much.

@f-f f-f merged commit 9fe8bfa into purescript:master Sep 21, 2020
@f-f
Copy link
Member

f-f commented Sep 21, 2020

Thank you 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants