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

Decode log messages on windows as UTF-8 #153

Closed
wants to merge 1 commit into from

Conversation

GardenTools
Copy link

On Windows svn (at least for current versions) pipes output encoded as UTF-8. I discovered this by having PySvn incur an exception when attempting to decode the output of log that had non-ascii punctuation characters in it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 78.035% when pulling ddc628b on GardenTools:master into 3784065 on dsoprea:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 78.035% when pulling ddc628b on GardenTools:master into 3784065 on dsoprea:master.

# subprocess.Popen defaults to locale.getpreferredencoding()
# which can be cp1252 (for example)
if (decode_text and platform.system() == "Windows"
and sys.version_info >= (3, 6)):
Copy link
Owner

Choose a reason for hiding this comment

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

This version seems arbitrary. It's not good practice to hard-code specific versions. If absolutely necessary, it's better to test for the features/characteristics that you actually need. In this case, you can just pass universal_newlines=True, which is supported in both 2.7 and 3.x .

Copy link
Owner

@dsoprea dsoprea May 1, 2020

Choose a reason for hiding this comment

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

Since we're already passing universal_newlines (

universal_newlines=decode_text,
), it will already always be returned as UTF-8 on Python 2.7 and string in Python 3.x in situations where text is expected. This pull-request is either in response to a not-understand problem or it's a symptom of a different problem. In which context are you encountering this?

Copy link
Owner

Choose a reason for hiding this comment

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

Please open an issue with your usage and the error that you're encountering and go from there.

@dsoprea
Copy link
Owner

dsoprea commented May 1, 2020

Closing this PR in lieu of an issue to discuss the problem. See comments above.

@dsoprea dsoprea closed this May 1, 2020
@GardenTools
Copy link
Author

Hi, I've raised #154

The version isn't arbitrary, it is the first version at which python allowed an encoding to be passed to Popen rather than selecting the default encoding itself - it isn't available in 3.5 https://docs.python.org/3.5/library/subprocess.html#subprocess.Popen

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