Windows behavior fixed, with appveyor tests passing (B) #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
August 26, 2018 19:44
jeremycarroll
changed the title
ekohl
reviewed
Aug 29, 2018
| from bumpversion.version_part import VersionPart, NumericVersionPartConfiguration, ConfiguredVersionPartConfiguration | ||
|
|
||
|
|
||
| if platform.system() == 'Windows' and sys.version_info[0] == 2: | ||
| def _command_args(args): |
There was a problem hiding this comment.
Would it make sense to introduce a subprocess.check_output wrapper that does this instead?
There was a problem hiding this comment.
That might be more readable, I was just going for as precise a change as needed.
| @@ -72,7 +81,7 @@ def commit(cls, message): | |||
| f.write(message.encode('utf-8')) | |||
| f.close() | |||
| env = os.environ.copy() | |||
| env['HGENCODING'] = 'utf-8' | |||
| env[str('HGENCODING')] = str('utf-8') | |||
| try: | |||
| subprocess.check_output(cls._COMMIT_COMMAND + [f.name], env=env) | |||
There was a problem hiding this comment.
Odd that this dosen't need encoding.
There was a problem hiding this comment.
f is a temporary file, and its name is ascii
|
Thanks @jeremycarroll -- merging this in. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This replaces my previous PR on this topic, with cleaned up commit history, and no longer and extra xfail.
This PR fixes issues on the Windows platform, particularly python2, revealed in the appveyor test failures.
In particular, encoding when making external calls with subprocess was usually incorrect.
Two specific changes on py2:
use strings not unicode to add to the environment before making subprocess calls
encode any non-ascii command line arguments (particularly →) in utf-8 before making the call.
A third change for both py2 and py3 is to change the file operations to text mode rather than binary mode, which simplifies handling of encoding.
In the tests, two further issues were addressed:
incorrect handling when hg (or presumably git) is absent (this issue was not visible on appveyor, but locally to me)
test_usage_string_fork changed to not explicit encode, and to use byte strings instead; xfailing in the most difficult case to fix: Windows on py3.
NB: I don't have a windows machine, so I am entirely reliant on appveyor for above analysis.
Concerning
test_usage_string_fork. This would fail in python3 on windows, except for the additional code for that case, which provides a.bumpversion.cfgfile which replaces the unicode character→with the ascii stringtoin the tag message and the commit message. Without this, I think there is a genuine defect (although I don't have a Windows machine) in which the-hoption attempts to print out this character as part of the help message and fails. Essentiallybumpversionassumes unicode support, and the workaround at least is to change those two strings explicitly in the.bumpversion.cfgor on the command line.