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

Upgrade all code to be Python3 only #5065

Merged
merged 45 commits into from Jan 21, 2019
Merged

Upgrade all code to be Python3 only #5065

merged 45 commits into from Jan 21, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 3, 2019

This PR drops Python 2 support and upgrade the code to be Python3 only:

  • removes py27 from Travis and tox
  • upgrade the code with pyupgrade that use super(), f-string and more (see official docs)
  • removes all mentions to from __future__ import etc
  • removes all import six mentions
  • removes usage of readthedocs/analytics/vendor/ipaddress.py and replaces it by https://docs.python.org/3/library/ipaddress.html
  • removes fullmatch custom function (see this PR)
  • removes six and future from requirements file
  • removes from django.utils import six
  • removes @pytest.mark.skipif(not six.PY2...
  • run pre-commit over all the files
  • when solving conflicts, probably always accepts "the other" and then re-run pyupgrade and pre-commit over the conflict files
  • check the usage of urlparse on #3683 once merged

This PR touches a lot of files. We should probably want to define what's the process we want to follow here but I wanted to have the PR done so we can see a diff and list all the things we need to do here.

pyupgrade has a way to be used as a pre-commit command so it will have effect only over the modified files in the PR. We are doing this already with yapf and we are still not covering all the code and complicating the review process. Although, we should consider this and discuss what's the path we want to follow.


IMPORTANT NOTE: this is huge PR that touches so many files. So, it's hard to review taking a look at the diff as a whole. You can click on the first commit and review one by one by clicking on "Next" excluding the ones that are script passes:

  • pyupgrade 1.11.0 on all code
  • isort all the files
  • Run pre-commit on all the files

Closes #4543

humitos added 7 commits Jan 3, 2019
pyupgrade --py3-plus --py3-only (*.py)
This file is generated by our code and can be used by Python2 when
building the documentation, so we need to keep the `u` before the
definition of the string.
@humitos humitos added this to the 3.0 milestone Jan 3, 2019
@humitos humitos requested a review from Jan 3, 2019
Copy link
Member

@stsewd stsewd left a comment

Just one test got changed incorrectly, but the rest looks good. Not sure why the search tests are failing



def validate_list(value):
"""Check if ``value`` is an iterable."""
if isinstance(value, (dict, string_types)):
if isinstance(value, (dict, (str,))):
Copy link
Member

@stsewd stsewd Jan 6, 2019

This should be (dict, str)

Copy link
Member Author

@humitos humitos Jan 8, 2019

Yeah, I suppose it was a mistake in the original code, and it should have been string_type (in singular).

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 6, 2019

Maybe we have a dependency in the search code that only runs on python2?

@humitos
Copy link
Member Author

@humitos humitos commented Jan 7, 2019

Maybe we have a dependency in the search code that only runs on python2?

I don't think so, otherwise the search wouldn't be working: we are running python3 in our production servers.

@humitos
Copy link
Member Author

@humitos humitos commented Jan 8, 2019

pyupgrade has a way to be used as a pre-commit command so it will have effect only over the modified files in the PR. We are doing this already with yapf and we are still not covering all the code and complicating the review process. Although, we should consider this and discuss what's the path we want to follow.

We discussed about this and we decided that we want to do it "all at once".

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 8, 2019

What about doing the autoformat in a separated PR? This was really large to review

@humitos
Copy link
Member Author

@humitos humitos commented Jan 8, 2019

Yes. I don't expect that nobody review what pyupgrade does, actually. I just checked some changes and I trusted on it.

I will create a PR with all the manual changes that I had to do, branching from this PR.

@humitos
Copy link
Member Author

@humitos humitos commented Jan 8, 2019

Or... at least, I will make commits that make sense separating what are automatic tools from manually touching the files.

@humitos humitos force-pushed the humitos/deprecate-python2 branch from f293a58 to 796d09b Jan 8, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Jan 9, 2019

This is ready for review: all tests and linting are passing.

  py36: commands succeeded
  lint: commands succeeded
  docs: commands succeeded
  congratulations :)

NOTE: py36 is randomly failing on travis (re-triggering without cache now)

We still have a problem with pre-commit because isort and yapf competes for the order of imports but also for the number of blank lines. As we prefer the behavior of isort we ran it after yapf when pre-commit is called without hooks ids.

In the end, running pre-commit multiple times at this point should not produce different outputs.

NOTE: I disabled add-trailling-comma because I got tired of the inconsistency that it produces 😁

I would be happy if we merge this sooner than later because otherwise we will be dealing with many of merge conflicts with the other PRs.

@humitos humitos requested a review from Jan 9, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Jan 9, 2019

Another note. I just tried black instead of yapf and I don't really like some decisions that it takes about the format of the code which, to me, it produces code that's visually harder than read that the rules that we have written in yapf.

Also, I found some of these in our entire repo:

INTERNAL ERROR: Black produced code that is not equivalent to the source.  Please report a bug on https://github.com/ambv/black/issues.  This diff might be helpful: /tmp/blk_dp6tr05z.log

@humitos
Copy link
Member Author

@humitos humitos commented Jan 21, 2019

All checks are passing locally. Although, Travis is doing something wrong with search tests and they are failing on connection :/

Copy link
Member

@ericholscher ericholscher left a comment

Let's do it. gulp

@humitos
Copy link
Member Author

@humitos humitos commented Jan 21, 2019

🙏

@humitos humitos merged commit 59d9e3e into master Jan 21, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/deprecate-python2 branch Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants