-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update obstore tests #2
Update obstore tests #2
Conversation
super().__init__(read_only=read_only) | ||
|
||
def __str__(self) -> str: | ||
return f"object://{self.store}" | ||
|
||
def __repr__(self) -> str: | ||
return f"ObjectStore({self!r})" | ||
return f"ObjectStore({self})" |
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.
This was changed to avoid a recursion error when __repr__
is called
if not isinstance( | ||
store, | ||
( | ||
obs.store.AzureStore, | ||
obs.store.GCSStore, | ||
obs.store.HTTPStore, | ||
obs.store.S3Store, | ||
obs.store.LocalStore, | ||
obs.store.MemoryStore, | ||
), | ||
): |
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.
@kylebarron I couldn't find an abstract base class or another way to simplify this, but suggestions for better approaches would be welcome
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.
Hmm, perhaps we should have a base class.
@@ -38,18 +38,29 @@ def __eq__(self, value: object) -> bool: | |||
if not isinstance(value, ObjectStore): | |||
return False | |||
|
|||
return self.store.__eq__(value.store) | |||
return bool(self.store.__eq__(value.store)) |
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.
This was added to avoid type errors with mypy
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.
Thank you! In the future we can update the isinstance
check when obstore adds a base class.
No description provided.