Skip to content
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

Add flag for specifying config path location #357

Merged
merged 9 commits into from
Aug 21, 2019
Merged

Add flag for specifying config path location #357

merged 9 commits into from
Aug 21, 2019

Conversation

justinwoo
Copy link
Contributor

@justinwoo justinwoo commented Aug 2, 2019

Description of the change

fixes #329, specify config path globally if desired for specific usages

e.g.

spago.dhall

-- ...
, sources =
    [ "codegen/**/*.purs", "src/**/*.purs", "test/**/*.purs" ]
}

i don't want to build with all the sources.

codegen.dhall

./spago.dhall // {
  sources = [
	"src/**/*.purs"
  ]
}
$ spago -x codegen.dhall build
# works fine

other uses:

"optional app separate from library"

"test config overrides and modifications"

"some other config modifications"

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 😊

@justinwoo
Copy link
Contributor Author

/bin/sh: psa: command not found

in osx build

@f-f
Copy link
Member

f-f commented Aug 5, 2019

@justinwoo that error there is because of the test harness and is a normal (I'll refactor the tests so it doesn't happen), but the tests fail because spago update-set is flaky on Travis machine (I'll put a PR up for this)

I'm working through my backlog, I'll review this later today

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.

This is cool by me, but I'm concerned about some edge cases that might be challenging UX-wise. A non-comprehensive list:

  • when we search for local packages we hardcode the spago.dhall. I think we should not change this, but it might be weird in some usecase I'm not considering
  • We have a few more mentions of the spago.dhall that we might want to fix too in Spago.Messages here and here and in Spago.Dhall here
  • when we encourage things like ./spago.dhall // { dependencies = [ "prelude" ] }, we have to make sure stuff that manipulates the AST directly still works (like spago install)
    So here I would like a test that makes sure that spago -x foo.dhall install simple-json works properly and:
    • actually adds the package if foo.dhall is ./spago.dhall // { dependencies = [ "prelude" ] }
    • and is a no-op if foo.dhall is ./spago.dhall // { sources = [ "src/**/*.purs" ] }

app/Spago.hs Outdated Show resolved Hide resolved
src/Spago/Prelude.hs Outdated Show resolved Hide resolved
Co-Authored-By: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>
@justinwoo
Copy link
Contributor Author

I think it would also be good to just disallow automatic editing for anything that doesn't conform to the expected Expr structure, and instead just print warnings on what the users should do manually with their frankenstein-ass configs

@f-f
Copy link
Member

f-f commented Aug 9, 2019

@justinwoo that is the current behaviour: if stuff doesn't conform to the expected structure then the editing is not performed. However we do not have the infrastructure to detect when this is the case and print warnings

TL;DR: I believe if you add the tests I mentioned they will just pass

@f-f
Copy link
Member

f-f commented Aug 20, 2019

This should now be good to go. @justinwoo can you take a look?

@f-f f-f changed the title add specifying config path location Add flag for specifying config path location Aug 20, 2019
@f-f f-f merged commit 13cf90c into master Aug 21, 2019
@f-f f-f deleted the config branch August 21, 2019 12:17
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.

Specify config path
2 participants