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

Fix unicode vcs changelog encode err #956

Conversation

davidlatwe
Copy link
Contributor

Problem

rez-release raise UnicodeDecodeError when releasing package with non-ascii characters in change log.

Solution

  • Explicitly set encoding="utf-8" when fetching results from vsc with subprocess. (but not if python<3.6)
  • Allow non-ascii character when formatting with json.

if sys.version_info[:2] >= (3, 6) and "encoding" in kwargs:
kwargs['encoding'] = 'utf-8'
if sys.version_info[:2] < (3, 6) and "encoding" in kwargs:
kwargs.pop("encoding")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is right. Looking at the original code, I think there may have been an existing bug, and L76 should be:

if sys.version_info[:2] >= (3, 6) and "encoding" not in kwargs:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my original intention was to not pin the encoding to "utf-8" (no specific reason), only removes it when the Python version is not compat, since encoding kwarg has been explicit set in commit d38e873, which should be removed if Python version incompat.

Now after #961, I think commit d38e873 should be reverted or it would be Python 2.7 incompat ?

@nerdvegas nerdvegas merged commit db95d8f into AcademySoftwareFoundation:master Sep 22, 2020
@davidlatwe davidlatwe deleted the fix-unicode-vcs-changelog-encode-err branch November 17, 2020 06:09
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.

2 participants