Implement ArbitraryOrd for relative::LockTime #2581
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tcharding
added
1.0
Pull Request Test Coverage Report for Build 8513797817Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
tcharding
force-pushed
the
03-13-arbitrary-ord
branch
from
2f0859c
to
11867db
Compare
apoelstra
previously approved these changes
Mar 14, 2024
tcharding
force-pushed
the
03-13-arbitrary-ord
branch
from
11867db
to
53c5d4f
Compare
|
Rebase only, no other changes. |
apoelstra
previously approved these changes
Mar 20, 2024
tcharding
force-pushed
the
03-13-arbitrary-ord
branch
from
53c5d4f
to
f5841e3
Compare
|
Forgot to comment yesterday, IIRC I fixed the rustdoc so its the same in both |
|
The comma after "incommensurate" should be a semicolon. |
|
You mean you want the sentence changed to this? /// Locktimes may be height- or time-based, and these metrics are incommensurate; there
/// is no total ordering on locktimes.Currently it is: /// Because locktimes may be height- or time-based, and these metrics are incommensurate, there
/// is no total ordering on locktimes(I repeatedly have to look up how to use semi-colons, I did so again last week.) |
|
Yep, exactly. |
TL;DR As we do for `absolute::LockTime` and for the same reasons; implement `ArbitraryOrd` for `relative::LockTime`. locktimes do not have a semantic ordering if they differ (blocks, time) so we do not derive `Ord` however it is useful for downstream to be able to order structs that contain lock times. This is exactly what the `ArbitraryOrd` trait is for. Update the rustdocs in `relative` and mirror the docs changes in `absolute`. Fix: rust-bitcoin#2566
tcharding
force-pushed
the
03-13-arbitrary-ord
branch
from
f5841e3
to
3520f55
Compare
|
Done! |
apoelstra
previously approved these changes
Mar 26, 2024
sanket1729
previously approved these changes
Apr 1, 2024
There was a problem hiding this comment.
ACK 3520f55. ordered should be added to all features flags doccomment listed inlib.rs
|
Can do, thanks for the review! |
Add "ordered" to the list of features in the `bitcoin` crate level docs.
|
Added as a separate patch, will need re-acking now please @sanket1729 |
apoelstra
approved these changes
Apr 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR As we do for
absolute::LockTimeand for the same reasons; implementArbitraryOrdforrelative::LockTime.locktimes do not have a semantic ordering if they differ (blocks, time) so we do not derive
Ordhowever it is useful for downstream to be able to order structs that contain lock times. This is exactly what theArbitraryOrdtrait is for.Fix: #2566