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.

Sign up for GitHub

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

smart_open/s3: Initial implementations of str and repr #359

Merged
merged 7 commits into from
Oct 6, 2019
Merged

smart_open/s3: Initial implementations of str and repr #359

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25b4cb3
smart_open/s3: Initial implementations of str and repr for SeekableBu…
zlatsic Oct 1, 2019
d0d533d
smart_open/s3: Follow-up on str and repr methods
Oct 2, 2019
899d314
Update smart_open/s3.py
zlatsic Oct 3, 2019
a440d10
smart_open/s3: Add closed parenthesis to the end of the repr function
Oct 3, 2019
c86674c
Merge branch 'user-friendly-s3' of github.com:ZlatSic/smart_open into…
Oct 3, 2019
3523dc8
smart_open/s3: Remove unneccessary line breaks, store input arguments…
Oct 4, 2019
ecaabd9
s3 - add comma to the end of multiline tuple
Oct 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions smart_open/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,14 @@ def truncate(self, size=None):
"""Unsupported."""
raise io.UnsupportedOperation

def __str__(self):
return "s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please use hanging indents?

return Foo(
    bar,
    baz,
    boz,
)

self._object.bucket_name, self._object.key)

def __repr__(self):
return "s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Suggested change
return "s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format(
return "smart_open.s3.SeekableBufferedInputBase(bucket_name='{}', key='{}')".format(

self._object.bucket_name, self._object.key)


class BufferedOutputBase(io.BufferedIOBase):
"""Writes bytes to S3.
Expand Down Expand Up @@ -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(
Copy link
Collaborator

Choose a reason for hiding this comment

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

str should prioritize brevity:

Suggested change
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).

self._object.bucket_name, self._object.key)

def __repr__(self):
return "s3.BufferedOutputBase(bucket_name='{}', key='{}')".format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, I think it makes sense to store them.

self._object.bucket_name, self._object.key)

def iter_bucket(bucket_name, prefix='', accept_key=None,
key_limit=None, workers=16, retries=3):
Expand Down