Skip to content

Remove Score Ord, PartialOrd, Eq and PartialEq impls #6420

Merged
mergify[bot] merged 4 commits intosigp:unstablefrom
jxs:remove-score-ord
Sep 25, 2024
Merged

Remove Score Ord, PartialOrd, Eq and PartialEq impls #6420
mergify[bot] merged 4 commits intosigp:unstablefrom
jxs:remove-score-ord

Conversation

@jxs
Copy link
Copy Markdown
Member

@jxs jxs commented Sep 20, 2024

Issue Addressed

following the conversation on #6407 (comment) this PR follows Rust std convention and removes the Ord PartialOrd Eq and PartialEq for Score which depends on f64. Instead as std impls total_cmp which handles NaN cases.
This also reverts #6407
cc @michaelsproul @eserilev

@jxs jxs marked this pull request as draft September 20, 2024 15:27
@jxs jxs force-pushed the remove-score-ord branch 3 times, most recently from dae7530 to f52f8c8 Compare September 20, 2024 16:40
@jxs jxs marked this pull request as ready for review September 20, 2024 17:11
michaelsproul
michaelsproul approved these changes Sep 23, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is nicer and should prevent misuse of a dodgy PartialEq/PartialOrd implementation.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. code-quality Networking labels Sep 23, 2024
@jxs
Copy link
Copy Markdown
Member Author

jxs commented Sep 25, 2024

@Mergifyio queue

@mergify
Copy link
Copy Markdown

mergify bot commented Sep 25, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 50d8375

@mergify mergify bot merged commit 50d8375 into sigp:unstable Sep 25, 2024
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
@jxs @chong-he
Remove Score Ord, PartialOrd, Eq and PartialEq impls (sigp#6420)
186d1f1 
* drop score Ord, PartialOrd, Eq and PartialEq impls

and impl total_cmp instead

* Revert "Fix test failure on Rust v1.81 (sigp#6407)"

This reverts commit 8a085fc.

* reverse in the compare function

* lint mdfiles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees

No one assigned

Labels

code-quality Networking waiting-on-author The reviewer has suggested changes and awaits thier implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jxs @michaelsproul