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 support for global REPL #280

Merged
merged 5 commits into from Jun 22, 2019

Conversation

@eahlberg
Copy link
Collaborator

commented Jun 17, 2019

Description of the change

Adds support for starting a REPL within a folder which has not been setup as a spago project. Fixes #168.

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)
Show resolved Hide resolved src/Spago/Build.hs Outdated
@eahlberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

@f-f I decided to take a look at this and I tried to implement it according to #168 (comment). However, I’m a bit unsure of a couple of things (in addition to the comment above):

  • Any idea for how this could be tested? I couldn’t find a test for spago repl, is there a specific reason for that?
  • I saw that PackageSet.ensureFrozen was called in Config.ensureConfig, which is not used for this, but I'm not sure what freezing a package set means. Should it be used here?
Show resolved Hide resolved src/Spago/Build.hs Outdated
Show resolved Hide resolved src/Spago/Build.hs Outdated
Show resolved Hide resolved src/Spago/Build.hs Outdated
Show resolved Hide resolved src/Spago/Config.hs Outdated
@f-f

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@eahlberg looks good so far! I added some minor comments, and to answer your other points:

  • let's skip tests for this one as I don't have ideas about how to test spago repl
  • "freezing" a Dhall file means to add sha256 hashes to the remote imports (the package set URL in our case). Since in this case we're initing a new project it should be a no-op as the template already has hashes, so it should be fine to use ensureConfig.
    We have a small section about freezing in the README, but it's very small as it's a detail that's supposed to be automated away from the user. However in there you can find a link to a longer description of Dhall's freezing mechanism
@f-f

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I just noticed that in addition to the install flags we'll also need to add the --package flag to add packages as specified in #168. It's going to look exactly as the packageNames argument:

packageNames = CLI.many $ CLI.arg (Just . PackageName) "package" "Package name to add as dependency"

..except it will be an opt instead of an arg

@eahlberg eahlberg force-pushed the eahlberg:global-repl branch from 18a5e4f to c57318b Jun 19, 2019

Show resolved Hide resolved CHANGELOG.md
Show resolved Hide resolved app/Spago.hs
Show resolved Hide resolved src/Spago/Build.hs Outdated

@eahlberg eahlberg force-pushed the eahlberg:global-repl branch from c57318b to 5031e5f Jun 19, 2019

@eahlberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2019

@f-f thanks for the feedback! 🙇‍♂️ I think I’ve addressed all of it and I also left a couple of questions above, so feel free to take a look when time permits.

@eahlberg eahlberg marked this pull request as ready for review Jun 19, 2019

@f-f

f-f approved these changes Jun 20, 2019

Copy link
Member

left a comment

Just a minor suggestion but otherwise looks great! 👏

I added you as collaborator so you can merge when you're ready 🙂

Show resolved Hide resolved src/Spago/Prelude.hs Outdated
Remove unused import
Co-Authored-By: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>
@eahlberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2019

@f-f cool, thanks!

@eahlberg eahlberg merged commit 02a9625 into spacchetti:master Jun 22, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@eahlberg eahlberg deleted the eahlberg:global-repl branch Jun 22, 2019

elliotdavies pushed a commit to elliotdavies/spago that referenced this pull request Jul 1, 2019

Add support for global REPL (spacchetti#280)
* Support global repl

* Update CHANGELOG

* Refactor

* Change flag name and use SomeException

* Remove unused import

Co-Authored-By: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.