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

Fix executable dir in + tabify pip & virtual envs guide #928

Merged
merged 4 commits into from
Jul 3, 2021

Conversation

ichard26
Copy link
Member

Fixes #783.

This patch also completes the "tabification" this document.

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Thanks for this; I like seeing the adoption of tabs in this doc. I've got a few questions/suggestions.


Afterwards, you should have the newest pip installed in your user site:
py -m pip --version
pip 9.0.1 from c:\python36\lib\site-packages (Python 3.6.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very old version of pip and Python. Can we take the opportunity to show the latest version for Windows and Unix/macOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is one of a few places in this document where the command and its output are shown in a single code-block, but with no distinction between them, which might lead folks to thinking that pip 9.0.1 ... is a command to execute.

If the output is valuable, I recommend putting into a separate code-block, maybe something like:

Suggested change
pip 9.0.1 from c:\python36\lib\site-packages (Python 3.6.1)
.. code-block:: text
pip 9.0.1 from c:\python36\lib\site-packages (Python 3.6.1)

Again, we'd want to do the same for Unix/macOS, and similarly for the "You can confirm you're in the virtual environment" instructions. But that could also be a follow-up PR.

Copy link
Member Author

@ichard26 ichard26 Jun 27, 2021

Choose a reason for hiding this comment

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

I was avoiding any informational changes to the document since I was afraid I lack the domain knowledge to do them properly, but this seems reasonable and I like it! I think it's fine to include these changes (on a first glance) since this PR already makes significant visual changes.

Copy link
Contributor

@bhrutledge bhrutledge Jun 27, 2021

Choose a reason for hiding this comment

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

Actually, I think the fact this PR makes significant visual changes is reason enough to do this in a subsequent PR. Let's just fix the slashes (below) for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created #931 as a follow-up.

Windows actually uses backslashes, not regular ol' slashes, oops :)

Co-authored-by: Brian Rutledge <brian@bhrutledge.com>
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Thanks!

@henryiii
Copy link
Contributor

There there a way to avoid the syntax highlighting doing this with the backslashes?

Screen Shot 2021-06-28 at 11 24 44 AM

@henryiii
Copy link
Contributor

henryiii commented Jun 28, 2021

I think you can use the .. code-block:: bat lexer. This problem happens on other pages, too (I think I used the wrong page for the screenshot above), not something to fix right now, but good idea for later. Created #934 for this.

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

One more small tweak, based on @henryiii's observation.

@bhrutledge bhrutledge requested a review from pradyunsg July 3, 2021 09:50
@pradyunsg pradyunsg merged commit d6521f9 into pypa:main Jul 3, 2021
@ichard26 ichard26 deleted the tabify-pip-and-virtual-environments branch July 4, 2021 21:44
@ichard26
Copy link
Member Author

ichard26 commented Jul 4, 2021

Thanks @henryiii, @pradyunsg and @bhrutledge for the reviews, it's well appreciated! 🙂

@bhrutledge
Copy link
Contributor

@ichard26 Thank for the improvements! If you feel like doing more, #931 would be a good follow-up.

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.

Small mistake in guide
4 participants