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

Refactor uses of dirutil.py to use the new default Unicode semantics #7604

Merged

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

commented Apr 22, 2019

Now that we finished the switchover in #7603 so that the dirutil functions default to Unicode, we can drop explicit arguments for simpler code.

The main motivations for using this default are:

  1. Default developers to using Unicode. The majority of the time, we want them using Unicode as a it's more predictable and the default in Py3. Making the decision for them means it's more likely they'll stick to the sensible default.
  2. Less cognitive overload due to a simpler API.
  3. Alignment with Python's open() functions.

@Eric-Arellano Eric-Arellano requested review from stuhood and cosmicexplorer Apr 22, 2019

@@ -209,12 +209,12 @@ def test_reset_interactive_output_stream(self):

with temporary_dir() as tmpdir:
some_file = os.path.join(tmpdir, 'some_file')
safe_file_dump(some_file, b'', mode='wb')
safe_file_dump(some_file, '')

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 22, 2019

Author Contributor

This is the only instance where I converted something that was previously bytes to be now unicode. It's safe to do here because we see the file is always created this same way (not parametrized) and just a few lines below we call read_file with Unicode.

The other usages were a bit less clear if it's safe, so I kept as is.

@jsirois
Copy link
Member

left a comment

I am not a fan of this, but as you say - it does mirror open defaults. I think the original sin was open itself not forcing an explicit mode to be passed in. This would have made conversion from 2-3 easier and everyone more aware of what we were doing.

I won't block.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I think the original sin was open itself not forcing an explicit mode to be passed in. This would have made conversion from 2-3 easier and everyone more aware of what we were doing.

Likely true, although worth pointing out that this is less of an issue in Python 3 than Python 2. Python 3 fails eagerly when mixing unicode and bytes, so it's far less likely developers will write code that messes up unicode vs. bytes. This contrasts with Py2 that freely mixes the two.

The main motivations I see for this are:

  1. Default developers to using Unicode. The majority of the time, we want them using Unicode as a it's more predictable and the default in Py3. Making the decision for them means it's more likely they'll stick to the sensible default.
  2. Less cognitive overload due to a simpler API.
  3. Alignment with Python's open() functions.

Still, acknowledged. Thanks for the review!

@Eric-Arellano Eric-Arellano merged commit 344ed7d into pantsbuild:master Apr 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:dirutil-default-unicode branch Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.