-
Notifications
You must be signed in to change notification settings - Fork 719
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
Remove orphan ToJSON instances that were moved to ledger #5772
Conversation
8446405
to
c272a12
Compare
cc @Jimbo4350 |
@neilmayhew> since the tests were not launched on the original instances: We don't really know if the instances from the ledger behave the same 🙃 Maybe remove the commit removing the local instances until the time when we'll be able to merge this PR? |
We do, because I ran them locally on a detached head. 😄 Also, my PR for ledger uses identical golden files (copied from this PR).
That would be unfortunate, because the point of this PR was to reduce the work needed when the time comes to integrate with a newer ledger, by making it easier to find the instances that need to be removed. I was assuming that whoever does the integration would do something similar to what I did above, by running the tests manually before and after the integration. However, perhaps the least confusing thing would be to split this PR into three:
If this is more acceptable to you, I'll create new PRs for (1) and (3) and update this one to become (2). |
c272a12
to
cf678bf
Compare
Yes I think this is the best way, to make sure nothing can go wrong during the integration. This procedures avoids the integrator to have to understand in details what's happening, he will anyway be safe! So I've approved #5775 and this one will serve as reference for the integration. Thanks @neilmayhew ❤️ |
cf678bf
to
ceedb1e
Compare
cc @Lucsanszky |
This PR is stale because it has been open 45 days with no activity. |
ceedb1e
to
7f697b6
Compare
, cardano-ledger-alonzo | ||
, cardano-ledger-allegra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the bounds can be removed now, because here are the versions on a recent version of master
(so where no bounds are specified):
→ ghc-pkg list | grep ledger-alonzo
cardano-ledger-alonzo-1.8.0.0
→ ghc-pkg list | grep ledger-allegra
cardano-ledger-allegra-1.4.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'll close this PR now.
Superseded by recent integration |
Description
These instances were added to ledger in IntersectMBO/cardano-ledger#4250. This PR shouldn't be merged until node is updated to use the corresponding versions of the ledger packages.
The golden tests pass with the current instances. They can be used to validate the new instances and should then be discarded (in #5776) because they will no longer be testing code that's in node.
Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7
Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.