Skip to content

Canonicalize version before comparing specifiers#283

Merged
pradyunsg merged 3 commits into
masterfrom
fix/282
Mar 26, 2020
Merged

Canonicalize version before comparing specifiers#283
pradyunsg merged 3 commits into
masterfrom
fix/282

Conversation

@di
Copy link
Copy Markdown
Member

@di di commented Mar 25, 2020

Fixes #282.

Copy link
Copy Markdown
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! The fix makes sense to me.

@di di requested a review from pradyunsg March 26, 2020 03:56
Comment thread packaging/specifiers.py
return NotImplemented

return self._spec == other._spec
return hash(self) == hash(other)
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.

Suggested change
return hash(self) == hash(other)
return (self._spec[0], canonicalize_version(self._spec[1])) == (other._spec[0], canonicalize_version(other._spec[1]))

seems cleaner to me than relying on hashes.

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.

Personally I like to rely on hash(), since it would be easier to add entries to _spec in the future (if this is ever needed) without the risk of forgetting to update both of them.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree with @uranusjr, I think this is actually cleaner and more clear. And since they're doing essentially the same thing, I don't see any issue with relying on hash here (although maybe I'm missing something).

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.

Well hash() computes a hash meaning you could have a hash collision between two completely different versions.

I'd find something like:

def _canonical_keys(self):
    return self._spec[0], canonicalize_version(self._spec[1])

def __eq__(self, other):
    return self._canonical_keys == other._canonical_keys

def __hash__(self):
    return hash(self._canonical_keys)

cleaner instead of relying on the huge hash space & hash randomization to avoid issues.

@pradyunsg pradyunsg merged commit 20a9a33 into master Mar 26, 2020
@pradyunsg pradyunsg deleted the fix/282 branch March 26, 2020 18:52
@pradyunsg
Copy link
Copy Markdown
Member

Thanks @di for the PR! Thanks @xavfernandez and @uranusjr for the reviews! I think it's perfectly fine to depend on hash. And and, thanks @brettcannon for his review as well!

I got to do the easy-and-fun part. ;)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Specifier equivalence check does not consider version equivalence

5 participants