Skip to content

Add Windows tab for some commands - #942

Merged
bhrutledge merged 13 commits into
pypa:mainfrom
dukecat0:add-windows-tab
Jul 13, 2021
Merged

Add Windows tab for some commands#942
bhrutledge merged 13 commits into
pypa:mainfrom
dukecat0:add-windows-tab

Conversation

@dukecat0

@dukecat0 dukecat0 commented Jul 12, 2021

Copy link
Copy Markdown
Member

Some commands are missing Windows tabs so I've added them.

Comment thread source/guides/installing-stand-alone-command-line-tools.rst Outdated
Co-authored-by: Brian Rutledge <brian@bhrutledge.com>
@dukecat0

Copy link
Copy Markdown
Member Author

I will use prompts when there are outputs of commands only.

Comment thread source/contribute.rst Outdated
nox using ``pip``::

python -m pip install --user nox
python3 -m pip install --user nox

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't look related to having windows tabs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I want to update python to python3 at the same time. However, is this should be fixed in another PR?

@webknjaz webknjaz Jul 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds like a separate change to me. The rule of thumb is to make PRs (and individual commits for that matter) atomic.
https://seesparkbox.com/foundry/atomic_commits_with_git

In particular, not following this practice makes things confusing for people who try to figure out what's going on here: the title says one thing, then the description may advertise another set of changes but the code differs even more. As a nice side-effect, small self-contained changes make it easier for reviewers to understand things and give feedback+merge faster.

If there's gazillion changes present, it encourages people to postpone the review "for later". Also, if there is a small change that could be approved right away, it'll still have to wait for all of the other unsuitable changes to be fixed first to be merged which is rather unfair.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here's a nice article on this topic: https://mtlynch.io/code-review-love/.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for these articles! :)
I will read them.

Comment thread source/guides/installing-stand-alone-command-line-tools.rst Outdated

$ pipx install PACKAGE
$ ENTRYPOINT_OF_PACKAGE [ARGS]
ENTRYPOINT_OF_PACKAGE [ARGS]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks weird. I would never have guessed that it's a placeholder for a command w/o a prompt. Dunno what to do about it...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Probably prompt should be remained here.

Comment thread source/guides/using-testpypi.rst Outdated
.. code::

$ twine upload --repository testpypi dist/*
twine upload --repository testpypi dist/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This prompt-removal crusade doesn't seem to be related to adding the windows tabs either

@dukecat0 dukecat0 Jul 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, but @bhrutledge started this here. 🤣

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. Still, let's try to keep PRs dedicated to a single cause each. Where it's possible, of course :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, and I will revoke these changes and open a new PR for these. :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apologies for the confusion. I thought it would be a small amount of extra scope, that would improve consistency. I see how it was too much. Thanks @meowmeowmeowcat and @webknjaz for bearing with me.

Comment thread source/guides/installing-stand-alone-command-line-tools.rst Outdated
@webknjaz
webknjaz requested a review from bhrutledge July 12, 2021 15:25
Now you can install packages with ``pipx install`` and access the package's entry point(s) from anywhere.
Now you can install packages with ``pipx install``::

$ pipx install PACKAGE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're keeping the prompt, the console lexer should be used.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, and this will be solved in another PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this was the only remaining unrelated change, I reverted it 307732b.

@bhrutledge
bhrutledge merged commit c3b6c46 into pypa:main Jul 13, 2021
@dukecat0
dukecat0 deleted the add-windows-tab branch July 13, 2021 03:41
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.

3 participants