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

Rename spago bundle and spago make-module (issue #147) #175

Merged
merged 7 commits into from
May 7, 2019

Conversation

Dretch
Copy link
Contributor

@Dretch Dretch commented Apr 27, 2019

spago bundle => spago bundle-app
spago make-module => spago bundle-module

I used bundle-app instead of bundle app for consistency with the existing command structure - i.e. currently we have psc-package-{local-setup,insdhall,clean} instead of psc-package {local-setup,insdhall,clean}.

I also avoided changing --main to --module because I don't have a view on what the best solution is there.

PR checklist:

  • Added a test for the contribution (if applicable)
  • Updated the version number in package.yaml according to SemVer (0.x.y.z, x changes on breaking changes, y on additions, z on patches)
  • Added some example of the new feature to the README

Fix #147

Dretch and others added 2 commits April 27, 2019 18:26
`spago bundle` => `spago bundle-app`
`spago make-module` => `spago bundle-module`
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.

@Dretch thanks, looks great!

I agree on keeping the --main flag, as it's called the same in pulp too. However, let's get the flag description closer to the pulp one, which is:

  --main -m <string>          Module to be used as the application's entry point. [Default: "Main"]

Since you also updated the README I ticked the remaining points of the checklist. The only thing remaining to fix are the tests that fail for some reason.
I just realized that "how to run the tests" is not documented anywhere except for the CI file, so I'll add a CONTRIBUTING file.

But how I usually run the tests is: stack install && cd tests/spago-test/ && ../spago-test.py

app/Main.hs Show resolved Hide resolved
@f-f
Copy link
Member

f-f commented Apr 29, 2019

@Dretch update on the tests: it looks like it was an issue with the caches on Travis, now everything seems to be fine

@Dretch
Copy link
Contributor Author

Dretch commented Apr 29, 2019

Thanks @f-f -- I had run the tests locally using the command you suggested (which I figured out by looking at the Travis build). As you say, It would be helpful if this was clearer/easier.

I will have a look at implementing the the other things you have mentioned. :)

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.

Looks great, thanks @Dretch! 👏

@f-f
Copy link
Member

f-f commented May 7, 2019

CI is not passing because the way we download dependencies is flaky. This should be fixed once and for all in #188, but this is good to merge

@f-f f-f merged commit 041884b into purescript:master May 7, 2019
elliotdavies pushed a commit to elliotdavies/spago that referenced this pull request Jul 1, 2019
* `spago bundle` => `spago bundle-app`
* `spago make-module` => `spago bundle-module`
* The old commands will error out with a suggestion to use the new ones
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.

Rename spago bundle and spago make-module
2 participants