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

Ensure audb.Repository is hashable #491

Merged
merged 4 commits into from
Dec 16, 2024
Merged

Ensure audb.Repository is hashable #491

merged 4 commits into from
Dec 16, 2024

Conversation

hagenw
Copy link
Member

@hagenw hagenw commented Dec 15, 2024

As audb.config.REPOSITORIES is given as a list of repositories, one would expect set(audb.config.REPOSITORIES) to work as well, which is not the case at the moment. It is solved in this pull request by adding a __hash__() method to audb.Repository.

Summary by Sourcery

Make audb.Repository hashable by implementing the hash method and add a test to ensure its hashability.

Enhancements:

  • Add hash method to audb.Repository to make it hashable.

Tests:

  • Add test to verify that audb.Repository objects are hashable.

Copy link
Contributor

sourcery-ai bot commented Dec 15, 2024

Reviewer's Guide by Sourcery

The PR implements hashability for the audb.Repository class by adding a __hash__() method that uses the string representation of the repository as the basis for hash calculation. This change allows Repository objects to be used in sets and as dictionary keys.

Class diagram for audb.Repository with hashability

classDiagram
    class Repository {
        +__eq__(other) bool
        +__hash__() int
        +__repr__() str
    }
    note for Repository "Added __hash__ method to enable hashability"
Loading

File-Level Changes

Change Details Files
Added hashability to Repository class
  • Implemented hash() method that returns hash of repository's string representation
  • Added test to verify Repository objects are hashable
  • Added test to verify Repository objects work in sets
audb/core/repository.py
tests/test_repository.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @hagenw - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The test case comparing set([repository]) to set([str(repository)]) is incorrect - these will never be equal since they contain different types. Consider testing set operations with Repository objects instead, e.g. set([repository]) == set([repository])
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

tests/test_repository.py Outdated Show resolved Hide resolved
tests/test_repository.py Outdated Show resolved Hide resolved
hagenw and others added 3 commits December 15, 2024 11:22
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Copy link
Member

@ChristianGeng ChristianGeng left a comment

Choose a reason for hiding this comment

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

I have one question:

when reading the code, I thought that you actually are about to implement __eq__.
I don't know what the default implementation of __eq__ is, but this post suggests that
it is ==.

Would it then not make sense to use __hash__ to calculate __eq__ in the same issue?

@hagenw
Copy link
Member Author

hagenw commented Dec 16, 2024

__eq__() was implemented before, and we use return str(self) == str(other). So, I guess your concern is that it should not be independent from __bash__() and we should better update it to return hash(self) == hash(other), correct?

@ChristianGeng
Copy link
Member

ChristianGeng commented Dec 16, 2024

__eq__() was implemented before, and we use return str(self) == str(other). So, I guess your concern is that it should not be independent from __bash__() and we should better update it to return hash(self) == hash(other), correct?

Ah so __eq__() exists. This makes it harder for me to comment as I do not know where the old implementation of __eq__ has been important and used in the past.

__str__ would get its value itself via __repr__, and __repr__ contains all necessary information (name, host, backend) to identify a repo. So I would not expect this to be of crucial importance any more.

Conceptually it was more intuitive to me to have return hash(self) == hash(other) when reading the code in the first place. But on the other hand it wouldn't buy much (and so I cannot see the need). So I am happy and will close with this comment.

I close the issue not the thread. Reopening.

@ChristianGeng
Copy link
Member

Sorry I closed the MR and not the thread. Reopened.

@hagenw
Copy link
Member Author

hagenw commented Dec 16, 2024

As you are ok with __eq__ is this fine to merge, or are there other things you might want to discuss?

@ChristianGeng
Copy link
Member

As you are ok with __eq__ is this fine to merge, or are there other things you might want to discuss?

Yes, using __repr__ to create equality here is fine. Equality is then module wise. Dependencies uses DataFrame equality to implement __eq__.

@hagenw hagenw merged commit 90c6b5f into main Dec 16, 2024
20 checks passed
@hagenw hagenw deleted the make-repo-hashable branch December 16, 2024 12:48
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