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

Projects
None yet
2 participants
@eahlberg
Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

Apparently, the tests pass on the CI 🤔

Refactor
- 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 eahlberg:report-all-missing-packages branch from 692a5a0 to 33d5baa Jun 15, 2019

@f-f

f-f approved these changes Jun 15, 2019

Copy link
Member

left a comment

@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

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 15, 2019

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

f-f added some commits Jun 15, 2019

@eahlberg eahlberg force-pushed the eahlberg:report-all-missing-packages branch from 06e2193 to 8ac1573 Jun 16, 2019

@eahlberg eahlberg force-pushed the eahlberg:report-all-missing-packages branch from 8ac1573 to b4579de Jun 16, 2019

@f-f f-f merged commit 11c1eb6 into spacchetti:master Jun 16, 2019

2 checks passed

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

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

Report all missing packages (spacchetti#264)
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
You can’t perform that action at this time.