Skip to content

Fix wrong behavior of Eq & Ord#36

Merged
hirrolot merged 6 commits into
teloxide:masterfrom
addap:fix-type-eq-ord
Jul 10, 2025
Merged

Fix wrong behavior of Eq & Ord#36
hirrolot merged 6 commits into
teloxide:masterfrom
addap:fix-type-eq-ord

Conversation

@addap

@addap addap commented Jul 9, 2025

Copy link
Copy Markdown
Contributor

I realized that the easiest way is to just rely on the derived implementations of the Eq and Ord traits.
To still get alphabetical sorting we must put the name field before the id field.
Added two tests that fail on master and pass now.

addap added 2 commits July 9, 2025 23:14
@addap
Add tests for wrong behavior of Eq & Ord
f9c811f 
As described in #35
@addap
Use derived Eq & Ord for Type
7c2eae1 
The previous handwritten implementations were not consistent with each
other as they allowed values a & b where a == b but a < b.
Fixes #35
@hirrolot hirrolot self-requested a review July 9, 2025 21:18
hirrolot
hirrolot requested changes Jul 10, 2025
View reviewed changes

@hirrolot hirrolot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, LGTM. Would you also add a CHANGELOG.md entry explaining your changes? Thanks!

addap added 4 commits July 11, 2025 00:54
@addap
Use rustfmt & reword comment
286669d
@addap
Add test for PartialEq for DependencyMap
204ac85 
The current implementation only zips the two key iterators together. So if
one of them is a subset of the other it only checks the keys in the
subset for equality. In the simplest case, the empty dependency map is
equal to every other dependency map.
@addap
Fix PartialEq implementation of DependencyMap
ed719f0 
Analogous to how equality is defined for BTreeMap, but only looking at keys.
@addap
Add changelog entry
042418b
@hirrolot hirrolot self-requested a review July 10, 2025 23:05
hirrolot
hirrolot approved these changes Jul 10, 2025
View reviewed changes

@hirrolot hirrolot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks again for figuring this out!

@hirrolot hirrolot merged commit 105af27 into teloxide:master Jul 10, 2025
1 check passed
@addap

addap commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

Okay, I just added an entry to the Changelog.

Also I was looking at other PartialEq implementations in the project and noticed that the one for DependencyMap might be wrong. When zipping together the two lists, if one of them is shorter than the other, the zip ends after the shorter list does not have any more items. As a consequence, only keys in the shorter list are checked for equality and therefore the empty dependency map is equal to every other. I assume this is wrong so I added a test & fix for that, too.

@hirrolot

hirrolot commented Jul 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

I've released v0.5.1 with your fix.

@addap addap deleted the fix-type-eq-ord branch January 24, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hirrolot hirrolot hirrolot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@addap @hirrolot