smart_open/s3: Initial implementations of str and repr #359
Conversation
…fferedInputBase and BufferedOutputBase
smart_open/s3.py
Outdated
| @@ -412,6 +412,14 @@ def truncate(self, size=None): | |||
| """Unsupported.""" | |||
| raise io.UnsupportedOperation | |||
|
|
|||
| def __str__(self): | |||
| return "s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format( | |||
There was a problem hiding this comment.
Can you please use hanging indents?
return Foo(
bar,
baz,
boz,
)
smart_open/s3.py
Outdated
| self._object.bucket_name, self._object.key) | ||
|
|
||
| def __repr__(self): | ||
| return "s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format( |
There was a problem hiding this comment.
I think it makes sense to include the full package name here. It'll be helpful for people outside of smart_open: they'll know what package the class comes from.
| return "s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format( | |
| return "smart_open.s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format( |
smart_open/s3.py
Outdated
| @@ -553,6 +561,13 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |||
| else: | |||
| self.close() | |||
|
|
|||
| def __str__(self): | |||
| return "s3.BufferedOutputBase(bucket_name='{}', key='{}')".format( | |||
There was a problem hiding this comment.
str should prioritize brevity:
| return "s3.BufferedOutputBase(bucket_name='{}', key='{}')".format( | |
| return "smart_open.s3.BufferedOutputBase(%r, %r)" % (self._object.bucket_name, self._object.key) |
Also please avoid using the .format() stuff (we don't like it).
smart_open/s3.py
Outdated
| self._object.bucket_name, self._object.key) | ||
|
|
||
| def __repr__(self): | ||
| return "s3.BufferedOutputBase(bucket_name='{}', key='{}')".format( |
There was a problem hiding this comment.
Please check out this link for an explanation of the difference between str and repr.
My understanding is that with repr, the user should be able to copy-paste the result into the Python interpreter and get back a copy of the original object. This may not always be possible, but this is something we should aim for. So, please include all the parameters passed to the constructor.
There was a problem hiding this comment.
Thank you for the feedback!
I've introduced most of the suggested changes, but I'd say that this one is still somewhat incomplete. As you said, sometimes these constructor parameters are not available and here we have a similar case where the values passed to the constructor are used to create other objects and the values themselves aren't stored anywhere.
I could try digging through the implementations of these other objects to find the values, but that seems hacky. Would it be OK if these parameters were stored in SeekableBufferedInputBase or BufferedOutputBase objects as in self._<parameter name>?
There was a problem hiding this comment.
Sure, I think it makes sense to store them.
Co-Authored-By: Michael Penkov <m@penkov.dev>
… user-friendly-s3
smart_open/s3.py
Outdated
| def __str__(self): | ||
| return "smart_open.s3.BufferedOutputBase(%r, %r)" % ( | ||
| self._object.bucket_name, | ||
| self._object.key |
There was a problem hiding this comment.
| self._object.key | |
| self._object.key, |
But really, I think the above should fit on one line and still be within limits.
… in the object, use them in repr fn
smart_open/s3.py
Outdated
| self._buffer_size, | ||
| self._line_terminator, | ||
| self._session, | ||
| self._resource_kwargs |
There was a problem hiding this comment.
Please add commas to multiline expressions, here and everywhere.
| self._resource_kwargs | |
| self._resource_kwargs, |
This way, if we want to add new parameters later, we only need to change one line, not two.
|
Great! Thanks for all the feedback, hope it wasn't too annoying going back and forth with the changes 😁 |
|
No problem at all. Hope you enjoyed the experience, if there's anything we can do to improve things for you as contributor, let us know, we'll take it on board. |
Initial implementations of str and repr for SeekableBufferedInputBase and BufferedOutputBase.
This wasn't too difficult to implement, so I've gone and made the first version before getting the answers to the questions at the issue thread. Please let me know if you'd like these outputs to look differently, if I should add other parameters to them or if I should add these to any other objects.