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

Resolve Issue #145, add warnings for Issue #146 #147

Merged
merged 4 commits into from
Nov 25, 2017
Merged

Resolve Issue #145, add warnings for Issue #146 #147

merged 4 commits into from
Nov 25, 2017

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented Nov 24, 2017

Issue 145: Added tests for reading and writing encoded text files on S3. Originally planned to add the same tests for the other subsystems (HDFS, WebHDFS, HTTP), but discovered that encoding is not handled at all there (it is ignored).

Issue 146: added warnings for cases where the encoding is specified and quietly ignored.

Assume that if mode is binary and encoding is explicitly specified, the
user actually wanted text.
@@ -157,20 +168,26 @@ def smart_open(uri, mode="rb", **kw):
elif parsed_uri.scheme in ("s3", "s3n", 's3u'):
return s3_open_uri(parsed_uri, mode, **kw)
elif parsed_uri.scheme in ("hdfs", ):
if kw.get('encoding') is not None:
warnings.warn('Issue #146: encoding is not supported for HDFS, ignoring')
Copy link
Owner

@piskvorky piskvorky Nov 24, 2017

Choose a reason for hiding this comment

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

Let's be a little more verbose. Why is it not supported -- is it a fault of the user, or a functionality gap in smart_open? What can the user do about this? (how to fix the warning?)

The issue number is too obscure too. If it's really relevant, we should quote it directly in the message, or at least provide a link. Conveying information to users in this way is too complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, done. Please have a look.

@menshikh-iv menshikh-iv changed the title Resolve Issue #145, add warnings for Issue #145 Resolve Issue #145, add warnings for Issue #146 Nov 25, 2017
@menshikh-iv menshikh-iv merged commit b512873 into piskvorky:master Nov 25, 2017
@menshikh-iv
Copy link
Contributor

Thank you @mpenkov 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants