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

Remove remaining `if PY{2,3}` snippets #7986

Merged
merged 4 commits into from Jul 1, 2019

Conversation

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

commented Jul 1, 2019

No description provided.

Eric-Arellano added some commits Jun 29, 2019

Remove IntegerForPid from osutil
This alias was needed in Py2 because of long, which no longer exists.

We could still keep an alias here, but for simplicity go with the normal `int`.

@Eric-Arellano Eric-Arellano requested review from stuhood, benjyw and blorente Jul 1, 2019

@@ -52,12 +50,6 @@ def known_os_names():
return reduce(set.union, OS_ALIASES.values())


if PY3:
IntegerForPid = (int,)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 1, 2019

Author Contributor

Open question: we could choose to keep this as a type alias. If so, we would want to tweak it to this:

Pid = int

Type aliases are totally valid and used in the MyPy / type hint world, per https://www.python.org/dev/peps/pep-0484/#type-aliases. I chose to get rid of it here because it seemed to create indirection and resulted in additional imports, but totally feasible and valid to add back in this modified form if you all would like.

This comment has been minimized.

Copy link
@blorente

blorente Jul 1, 2019

Contributor

I would love to keep it as a type alias, in particular because in the realm this is used integers mean two very different things: File descriptors and pids. So being explicit for which of the two this represents would help a lot.

However, until we typecheck the code that uses it, I'm not sure it's worth having, so I would settle for a TODO(#issue) for this one.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 1, 2019

Author Contributor

I'll add back! It's easy to do, even without fully type checking the code.

@blorente
Copy link
Contributor

left a comment

Thanks a lot :)

@unittest.skipIf(PY3, reason='Print statement check disabled on Python 3 because interpreter '
'will already throw a syntax error.')
# TODO(#7979): Rework tests so that we can run this with Python 2.
@unittest.skip

This comment has been minimized.

Copy link
@blorente

blorente Jul 1, 2019

Contributor

Could we remove this test altogether? If the users can run pants under python2, we should still only skipif(PY3. If the user can't run this under Python 2, there's not much sense keeping this around.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 1, 2019

Author Contributor

No, we should not remove it. This code under contrib.python.checks.checker is special in that it may be ran under Py2 or Py3, depending on what select-interpreter chooses.

@@ -52,12 +50,6 @@ def known_os_names():
return reduce(set.union, OS_ALIASES.values())


if PY3:
IntegerForPid = (int,)

This comment has been minimized.

Copy link
@blorente

blorente Jul 1, 2019

Contributor

I would love to keep it as a type alias, in particular because in the realm this is used integers mean two very different things: File descriptors and pids. So being explicit for which of the two this represents would help a lot.

However, until we typecheck the code that uses it, I'm not sure it's worth having, so I would settle for a TODO(#issue) for this one.

@@ -121,8 +120,7 @@ def test_collection_ordering(self):
self.assertEqual(self._coercing_json_encode({2, 1, 3}), '[1, 2, 3]')
self.assertEqual(self._coercing_json_encode({'b': 4, 'a': 3}), '{"a": 3, "b": 4}')
self.assertEqual(self._coercing_json_encode([('b', 4), ('a', 3)]), '[["b", 4], ["a", 3]]')
self.assertEqual(self._coercing_json_encode([{'b': 4, 'a': 3}]),
'[{"b": 4, "a": 3}]' if PY3 else '[{"a": 3, "b": 4}]')
self.assertEqual(self._coercing_json_encode([{'b': 4, 'a': 3}]), '[{"b": 4, "a": 3}]')

This comment has been minimized.

Copy link
@blorente

blorente Jul 1, 2019

Contributor

There must be a fun story behind this one :) Would love to hear it if you have some time. Good change though.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 1, 2019

Author Contributor

Dictionaries in Python 2 are deterministic but only as an implementation detail and not in a particular order. In Python 3.6+, they are deterministic by insertion order. This was the cause of a whole bunch of test failures during the migration :)

@Eric-Arellano Eric-Arellano merged commit 8833bae into pantsbuild:master Jul 1, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:remove-remaining-py2 branch Jul 1, 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.