-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-4834 Add __repr__ to IndexModel, SearchIndexModel #1909
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
Conversation
There are quite a lot of possible IndexModel kwargs besides the ones hardcoded here. Looking at |
Thanks, fixed. I'm going to add some tests for the other known kwargs. |
Ruff wants you to use f-string or .format() instead:
|
Fair enough, ruff 👍 ca34322 |
Thanks @timgraham and @ShaneHarvey, I think this is ready to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides some bikeshedding I won't speak definitively about.
Thanks! May as well bikeshed while we are in here. I have implemented your suggestions as well as moved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a note in the doc/changelog.rst? Here's an example:
- Added :func:`repr` support to :class:`bson.tz_util.FixedOffset`.
@@ -773,6 +773,19 @@ def document(self) -> dict[str, Any]: | |||
""" | |||
return self.__document | |||
|
|||
def __repr__(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruff is still failing because it wants you to use f-strings. Do you have pre-commit set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, forgot my pre-commit install
! I think I just need to add a more meaningful test to the SearchIndexModel
test class to finish this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid force pushing because it makes it harder to review what's changed on the new commits.
- Review fixes
For use with django-mongodb e.g.