Skip to content
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

Implement Eq and Debug traits #21

Merged
merged 7 commits into from
Jan 24, 2020
Merged

Implement Eq and Debug traits #21

merged 7 commits into from
Jan 24, 2020

Conversation

Stupremee
Copy link
Contributor

@Stupremee Stupremee commented Jan 24, 2020

This pr implements the PartialEq, Eq and the Debug traits.
These additions were requested in #10

@codecov
Copy link

codecov bot commented Jan 24, 2020

Codecov Report

Merging #21 into master will decrease coverage by 1.64%.
The diff coverage is 66.66%.

Impacted Files Coverage Δ
src/lib.rs 81.59% <66.66%> (-2.35%) ⬇️

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/basic.rs Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Jan 24, 2020

I think it would be good to call out in the documentation (probably under "Consistency") the fact that checking for map equality might yield weird results if the map is being concurrently modified!

@Stupremee
Copy link
Contributor Author

Tests are failing because the len check is missing in PartialEq trait

src/lib.rs Outdated Show resolved Hide resolved
@Stupremee
Copy link
Contributor Author

I recreated your idea here and it doesn't work.

@Stupremee
Copy link
Contributor Author

I will just leave it as it is and we can implement it later if possible.
Unless you find a solution

@jonhoo
Copy link
Owner

jonhoo commented Jan 24, 2020

Ah, that is too bad. Well, since the current Index impl doesn't work either, I think we should then just remove the impl of Index for the time being :)

@Stupremee Stupremee changed the title Implement Eq and Index methods Implement Eq and Debug traits Jan 24, 2020
@Stupremee
Copy link
Contributor Author

Okay. I will just add Debug impl too

@Stupremee Stupremee marked this pull request as ready for review January 24, 2020 18:15
tests/basic.rs Outdated Show resolved Hide resolved
@jonhoo jonhoo merged commit d3706b2 into jonhoo:master Jan 24, 2020
@jonhoo
Copy link
Owner

jonhoo commented Jan 24, 2020

Excellent, thank you!

@jonhoo jonhoo mentioned this pull request Jan 24, 2020
@Stupremee Stupremee deleted the eq-index-clear-implementation branch January 24, 2020 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants