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

Python 3 Upgrade #1683

Merged
merged 11 commits into from
Nov 23, 2020
Merged

Python 3 Upgrade #1683

merged 11 commits into from
Nov 23, 2020

Conversation

nikhiljohn10
Copy link
Contributor

Draft for Python 3 upgrade

  • Added python versions to book.json
  • Converted python versions to book variable

If this draft is acceptable, i'll create a PR using these commits.

@das-g @ekohl Please review. Thanks

@nikhiljohn10

This comment has been minimized.

@nikhiljohn10 nikhiljohn10 marked this pull request as ready for review November 13, 2020 07:28
@nikhiljohn10 nikhiljohn10 requested a review from a team as a code owner November 13, 2020 07:28
@nikhiljohn10 nikhiljohn10 marked this pull request as draft November 13, 2020 07:29
@das-g
Copy link
Member

das-g commented Nov 13, 2020

1.

  • Do I have to create screenshots for the new upgrade within this PR regarding Python?
    -or-
  • Should I create a new PR will new screenshots of every screenshots found inside tutorial?

Whichever you prefer. Third option:

  • File an issue about updating the screenshots, so someone else can do it.

That's the idea behind preparing the updates that depend on each other on a separate branch: We don't have to cover everything in a single PR, and more than one person can work on different aspects.

@das-g
Copy link
Member

das-g commented Nov 13, 2020

2.

  • Manually hard code version of python in tutorial?
    -or-
  • Automate Python versions using book.json and variable replacement?

Automating it with the variable might be a good idea, especially to keep the lesser-active translation more in sync. Though, it can result in these translations getting internally inconsistent, if stuff that depends on the version but hasn't been automated doesn't get re-translated.

Would 4863b61 already cover all occurrences of the Python version number(s)?

@das-g
Copy link
Member

das-g commented Nov 13, 2020

3.

  • Does python intro need any changes according to Python 3.8 version?
    -or-
  • Should I not touch the python Intro tutorial at all?

As far as I know, Python 3.8 and 3.9 are fully backwards-compatible to the Python version(s) we currently use, i.e., everything that will work in Python 3.6 should work in 3.8 and 3.9, too. So there is no need to update Python intro. (We should probably test it with the new Python version(s), but I don't expect to find any differences in behavior except maybe for slightly different output formats here and there.)

While I very much like the newly introduced features of the newer Python versions, I don't think any of them are relevant enough for newcomers that they'd have to be covered in the Intro. If we ever come to a different conclusion, we can still make that change then, in a new PR.

@nikhiljohn10

This comment has been minimized.

@nikhiljohn10 nikhiljohn10 marked this pull request as ready for review November 13, 2020 10:09
@nikhiljohn10
Copy link
Contributor Author

On file django_installation/instructions.md, Line 176:

(myvenv) ~$ python -m pip install --upgrade pip

Can simply use pip install -U pip instead? I have tested and both commands result in upgrading pip inside myvenv directory.

@das-g
Copy link
Member

das-g commented Nov 13, 2020

On file django_installation/instructions.md, Line 176:

(myvenv) ~$ python -m pip install --upgrade pip

Can simply use pip install -U pip instead? I have tested and both commands result in upgrading pip inside myvenv directory.

I'm not sure. The python3 -m part was added in #1274 (and changed to the python -m variant without version number in #1450). @anapaulagomes, @ekohl can you remember what the problem was with plain pip install --upgrade pip instead of python -m pip install --upgrade pip?

IIRC the python -m pip install --upgrade pip variant proved to work on more different system (various Linux distributions, probably) than plain pip install --upgrade pip. Why that might have been, I'm not quite sure, as within a Python 3 virtualenv, the pip command should always be available, whether pip installed system-wide or not. (Some Linux distributions package pip separately from Python, thus it's possible that system-wide, Python 3 is installed, but the pip for it isn't. But in a virtualenv, that shouldn't be an issue.)

@das-g
Copy link
Member

das-g commented Nov 13, 2020

On file django_installation/instructions.md, Line 176:

(myvenv) ~$ python -m pip install --upgrade pip

Can simply use pip install -U pip instead? I have tested and both commands result in upgrading pip inside myvenv directory.

As for -U vs. --upgrade: Although it's more to type, I'd stick with the long option, as it's more obvious what it might do than with the short one, for which a new user would probably have to consult the documentation.

@nikhiljohn10
Copy link
Contributor Author

As for -U vs. --upgrade: Although it's more to type, I'd stick with the long option, as it's more obvious what it might do than with the short one, for which a new user would probably have to consult the documentation.

I don't find that as a valid statement since there are other short forms of options used inside tutorial. For eg, -r, --requirement in pip install, -u, --set-upstream in git push etc. When I started learning python, i have also google all these to understand what they really do.

We can add a 'NOTE' under pip install -U p about its -U option is --upgrade shortform.

How about that?

@nikhiljohn10

This comment has been minimized.

@anapaulagomes
Copy link
Contributor

Sorry, I don't remember. 😐

@das-g
Copy link
Member

das-g commented Nov 14, 2020

Would 4863b61 already cover all occurrences of the Python version number(s)?

@das-g Yes, it covers 100% of version texts inside the tutorial and converts to the GitBook variable.

👍

Does that mean to commit in this draft PR is ok approval?

I'm not sure I understand your question. Can you rephrase it? (If English isn't your native language, maybe write it in your native language.)

If yes, I don't need a new PR. I can change the draft status of this PR for review and merge. There is nothing else to update regarding python versions.

Yes, converting a draft PR to a normal, merge-able PR is completely OK. You don't have to file a new PR for the same thing.

@das-g
Copy link
Member

das-g commented Nov 14, 2020

We can add a 'NOTE' under pip install -U p about its -U option is --upgrade shortform.

Yeah, with such an explanation (be it in a note admonition or just in the prose) it should be fine. Feel free to open a separate PR for that. (Separate, as it's independent from the python version change.)

@das-g
Copy link
Member

das-g commented Nov 14, 2020

About python -m pip ... vs. just pip ...:

I don't think it was a Windows specific issue. On Windows, too, a virtualenv created by python -m venv should include pip. And when the virtualenv is activated, the pip from the virutalenv should be available as simply pip.

After some research, I guess the real reason is, that Python 3's venv module didn't always install pip:

(Though I'm not quite sure how the pip module (which is used by -m pip) could have been available then, as it isn't a standard library module, and wasn't one in Python 3.3, either.)

Since Django 3.1 requires Python ≥ 3.6 we can probably drop the workaround for all operating systems. (Though, we should test on all of them, first.)

@nikhiljohn10
Copy link
Contributor Author

nikhiljohn10 commented Nov 14, 2020

About python -m pip ... vs. just pip ...:

I don't think it was a Windows specific issue. On Windows, too, a virtualenv created by python -m venv should include pip. And when the virtualenv is activated, the pip from the virutalenv should be available as simply pip.

After some research, I guess the real reason is, that Python 3's venv module didn't always install pip:

(Though I'm not quite sure how the pip module (which is used by -m pip) could have been available then, as it isn't a standard library module, and wasn't one in Python 3.3, either.)

Since Django 3.1 requires Python ≥ 3.6 we can probably drop the workaround for all operating systems. (Though, we should test on all of them, first.)

Hope this helps

Packaging status

Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

👍 Looks good, except for some details:

book.json Outdated Show resolved Hide resolved
en/python_installation/instructions.md Outdated Show resolved Hide resolved
en/python_installation/instructions.md Outdated Show resolved Hide resolved
en/python_installation/instructions.md Outdated Show resolved Hide resolved
@das-g
Copy link
Member

das-g commented Nov 14, 2020 via email

Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

Some URLs point to specific Python versions other than they should refer now.

Should we have variables for them, too, so that they can be changed at the same place as the versions themselves?

en/python_installation/instructions.md Outdated Show resolved Hide resolved
en/python_installation/instructions.md Outdated Show resolved Hide resolved
@nikhiljohn10
Copy link
Contributor Author

I have generated a stagging site under my personal domain. This contains all the above commits.

https://django.nikz.in/en/

Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

I noticed another thing:

en/python_installation/instructions.md Outdated Show resolved Hide resolved
Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@das-g das-g requested a review from ekohl November 22, 2020 23:01
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks good and nice that it'll be easier in the future.

@ekohl
Copy link
Collaborator

ekohl commented Nov 23, 2020

@das-g shall we merge it or is there anything else to do?

@das-g
Copy link
Member

das-g commented Nov 23, 2020

@das-g shall we merge it or is there anything else to do?

I'm not aware of anything more that would have to be done for changing the Python version(s).

According to its release notes, the currently used Django version (2.2) does support both Python versions the tutorial will mention with this PR: Python 3.6 & Python 3.8. Thus it should be fine to merge this right into master instead of (or maybe better, additionally to) merging it into django-3.1.

I didn't yet have time to test the complete tutorial with any of the new Python versions, and probably won't have time for that in the near future. So if anyone did test it or will test it, please comment here. Nonetheless, I'm confident that this shouldn't introduce any major problems.

Shall I go ahead an merge it, @ekohl ? (Or, if you agree, feel free to merge it yourself.)

@ekohl
Copy link
Collaborator

ekohl commented Nov 23, 2020

I didn't yet have time to test the complete tutorial with any of the new Python versions, and probably won't have time for that in the near future. So if anyone did test it or will test it, please comment here. Nonetheless, I'm confident that this shouldn't introduce any major problems.

I'm in a similar position so I was also a bit hesitant to merge. However, like you I'm also confident it won't introduce major problems. Let's merge it.

Thanks @nikhiljohn10!

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.

4 participants