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

feat: set __str__ to use name and id for Dataset and Model #1732

Merged
merged 15 commits into from
Jan 23, 2024

Conversation

AdriMarteau
Copy link
Contributor

Description

Define __str__ for the Dataset and Model classes for more user friendly prints and reports

Related Issue

Original discussion

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

Copy link
Member

@kevinmessiaen kevinmessiaen 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 the work, I only have one remark is that the type of object is not visible:
test-b55675f8-2ffd-4d7e-92b8-945e40be5b68 can be either a Model or a Dataset.

I believe something like that would be better:
<giskard.Model name="test" uuid=b55675f8-2ffd-4d7e-92b8-945e40be5b68>

@AdriMarteau
Copy link
Contributor Author

Thanks for the work, I only have one remark is that the type of object is not visible: test-b55675f8-2ffd-4d7e-92b8-945e40be5b68 can be either a Model or a Dataset.

I believe something like that would be better: <giskard.Model name="test" uuid=b55675f8-2ffd-4d7e-92b8-945e40be5b68>

I overloaded the __str__ method for the moment. I can also add a __repr__ method that with the details you are requesting. I found example like this telling but I am happy to change my implementation if it's more relevant

@AdriMarteau
Copy link
Contributor Author

@kevinmessiaen I have added utest for __repr__ to show that we have richer string vs the naming with __str__.
But if you thing that it is better to also include the class name in __str__, I can add it

@kevinmessiaen
Copy link
Member

@kevinmessiaen I have added utest for __repr__ to show that we have richer string vs the naming with __str__. But if you thing that it is better to also include the class name in __str__, I can add it

I don't have a strong opinion on it so we can go without it. But I'm not convinced by having name-uuid, something like name (uuid) might be easier to read IMO

@AdriMarteau
Copy link
Contributor Author

@kevinmessiaen I have added utest for __repr__ to show that we have richer string vs the naming with __str__. But if you thing that it is better to also include the class name in __str__, I can add it

I don't have a strong opinion on it so we can go without it. But I'm not convinced by having name-uuid, something like name (uuid) might be easier to read IMO

Very good point let me update this request

@kevinmessiaen kevinmessiaen merged commit 35b24e7 into Giskard-AI:main Jan 23, 2024
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants