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

Report all missing packages #264

Merged
merged 8 commits into from
Jun 16, 2019

Conversation

eahlberg
Copy link
Contributor

@eahlberg eahlberg commented Jun 14, 2019

Description of the change

If there are multiple dependencies which do not exist in the package set, instead of just reporting the first missing package, all missing packages will be reported which fixes #223. In the same manner, all circular dependency errors will be reported.

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/Packages.hs Outdated Show resolved Hide resolved
src/Spago/Packages.hs Outdated Show resolved Hide resolved
@eahlberg
Copy link
Contributor Author

eahlberg commented Jun 14, 2019

@f-f thanks for the intro and help earlier today! If you (or anyone else) has feedback about the code in the PR, it's really appreciated! Regarding the correctness, I'm still confused why the tests fail when it works locally by running spago build in a folder as described in #223, but I'll try to dig into that some more.

@eahlberg
Copy link
Contributor Author

eahlberg commented Jun 14, 2019

Apparently, the tests pass on the CI 🤔

- FoldMap once instead of three times, and remove unused functions
- Transform sets and maps to lists at the same level
- Change argument order to make things consistent
- Remove type definitions for functions within where clauses
- Improve error message
@eahlberg eahlberg force-pushed the report-all-missing-packages branch from 692a5a0 to 33d5baa Compare June 15, 2019 07:27
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.

@eahlberg thanks for doing this, looks great! 👏

If you'd like to setup some more tests where we match the output of the command we can rebase this on #265 that makes it much easier to add new tests

@eahlberg
Copy link
Contributor Author

Alright, so I got a bit further with this:

  • the tests now pass locally without any change (that I'm aware of) from my side, so maybe it was some kind of environment issue 🤷‍♂️
  • I also refactored some of the ugly parts
  • I'm a bit unsure whether this warrants updating or adding some test case, so that'd be good if someone could take a look at

@eahlberg eahlberg force-pushed the report-all-missing-packages branch from 06e2193 to 8ac1573 Compare June 16, 2019 09:47
@eahlberg eahlberg force-pushed the report-all-missing-packages branch from 8ac1573 to b4579de Compare June 16, 2019 09:58
@f-f f-f merged commit 11c1eb6 into purescript:master Jun 16, 2019
elliotdavies pushed a commit to elliotdavies/spago that referenced this pull request Jul 1, 2019
If there are multiple dependencies which do not exist in the package set instead of just
reporting the first missing package, all missing packages will be reported.
In the same manner, all circular dependency errors will be reported.
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.

Report all missing packages together
2 participants