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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip extra property when comparing LocatedSpanEx #46

Closed
wants to merge 5 commits into from
Closed

Skip extra property when comparing LocatedSpanEx #46

wants to merge 5 commits into from

Conversation

zakcutner
Copy link
Contributor

LocatedSpanEx includes an extra property that is designed for "extra information that can be embedded by the user".

Since PartialEq is derived on LocatedSpanEx, checking equality on two spans involves checking equality on the extra property. This is undesirable as, by definition, the extra information is separate from the input so should not be compared when determining if two input spans are the same.

I have come across this issue as there are several nom combinators which require comparing two inputs, for instance many1. Currently, the only workaround is wrapping the extra information in a trivial struct which always returns true when compared.

I resolve this by instead providing a custom PartialEq implementation for LocatedSpanEx. Whilst doing this, I also fixed a typo within the docs for the extra property and implemented Eq (equivalence relation) on LocatedSpanEx where the generic T allows for this. 馃槃

I hope this helps, let me know if there are any questions or suggestions!

Copy link
Collaborator

@progval progval left a comment

Choose a reason for hiding this comment

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

I thought we were already doing that. Thanks!

Could you add a test to avoid regressions in the future?

src/lib.rs Outdated Show resolved Hide resolved
@progval progval removed the request for review from fflorent January 14, 2020 18:11
@zakcutner
Copy link
Contributor Author

I've added a test to make sure the extra property is not compared for PartialEq

@progval
Copy link
Collaborator

progval commented Jan 14, 2020

Thanks, merged! (be80026 and 887f7b4)

@progval progval closed this Jan 14, 2020
@zakcutner
Copy link
Contributor Author

Perfect, thanks for getting this merged so quickly 馃槏

Any idea when this change will get released to crates.io?

@progval
Copy link
Collaborator

progval commented Jan 14, 2020

@fflorent Could you publish a minor release for this?

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.

None yet

2 participants