-
Notifications
You must be signed in to change notification settings - Fork 93
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
support hashable-1.4.3 #1717
support hashable-1.4.3 #1717
Conversation
Left BadAdjacents -> do | ||
putMVar cmv c | ||
return False |
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.
Before the function would fail with an userError
in this case. This change informs the caller when mining fails.
In practice there are tests where it can happen that the mining of a blocked chain is attempted. It was by pure chance that this didn't showed up before. The latest version of hashable
changed the hash value of chain ids and thus the ordering of chains in tests, which caused this issue to surface.
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.
What do you think about applying a "if comments on github make it easy to understand the PR, they should be in the code" rule here?
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 am not sure in case of this comments, since they talk an old version of the code and the diff with the new version. I think, PR comments my be a better place for those.
But, generally, I agree, that comments that explain the behavior of the new code should be code comments.
cid = unsafeChainId 9 | ||
-- several tests in this file expect chain 9 |
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.
With the new hashable version the value of someChainId
changes. This fixes the previous value.
someChainIdAt v h = minimum $ chainIdsAt v h | ||
-- guaranteed to succeed because the empty graph isn't a valid chain graph. |
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.
We could have fixed the value to value with the previous version of hashable
, which would have avoided updating of golden files. But using the minimum
seems like a more reasonable and intuitive default.
@@ -344,7 +344,7 @@ constraints: any.Boolean ==0.2.4, | |||
any.time-compat ==1.9.6.1, | |||
time-compat -old-locale, | |||
any.time-manager ==0.0.0, | |||
any.tls ==1.7.0, | |||
any.tls ==1.7.1, |
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.
this update is unrelated, but nice to have.
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.
LGTM 👍
Left BadAdjacents -> do | ||
putMVar cmv c | ||
return False |
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.
What do you think about applying a "if comments on github make it easy to understand the PR, they should be in the code" rule here?
The updates the golden files are caused to changes in the test harness and don't represent on-chain semantics.
Tested that the changes also work with hashable<1.4.3.