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
Merged

Conversation

eahlberg
Copy link
Contributor

@eahlberg eahlberg 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)

src/Spago/Build.hs Outdated Show resolved Hide resolved
@eahlberg
Copy link
Contributor Author

@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?

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 Show resolved Hide resolved
@f-f
Copy link
Member

f-f 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
Copy link
Member

f-f 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:
https://github.com/spacchetti/spago/blob/92e9c64bdde0373a64514def96b7f78738601909/app/Spago.hs#L158
..except it will be an opt instead of an arg

CHANGELOG.md Show resolved Hide resolved
app/Spago.hs Show resolved Hide resolved
src/Spago/Build.hs Outdated Show resolved Hide resolved
@eahlberg
Copy link
Contributor Author

eahlberg 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 June 19, 2019 20:40
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.

Just a minor suggestion but otherwise looks great! 👏

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

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

@f-f cool, thanks!

@eahlberg eahlberg merged commit 02a9625 into purescript:master Jun 22, 2019
@eahlberg eahlberg deleted the global-repl branch June 22, 2019 15:15
elliotdavies pushed a commit to elliotdavies/spago that referenced this pull request Jul 1, 2019
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global spago repl
2 participants