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

Add inline tabs on Increase Test Coverage page for commands on different systems #1228

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

lancegoyke
Copy link
Contributor

@lancegoyke lancegoyke commented Nov 18, 2023

This PR should close a single TODO in #1196.


📚 Documentation preview 📚: https://cpython-devguide--1228.org.readthedocs.build/testing/coverage/

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

The rest of the page has ./python X and make X commands that we tabify to help macOS/Windows users copy and paste the right one.

And let's also add the JS.

testing/coverage.rst Show resolved Hide resolved
testing/coverage.rst Show resolved Hide resolved
@lancegoyke
Copy link
Contributor Author

@hugovk

And let's also add the JS.

Thank you for the explicit repetition on this instruction.

The rest of the page has ./python X and make X commands that we tabify to help macOS/Windows users copy and paste the right one.

I just want to clarify something before adding all of the tabs. There's a line earlier in the doc after installing coverage that reads: "You can now use python without the ./ for the rest of these instructions, as long as your venv is activated."

Should we change all of these commands to python rather than their tabbed ./, .exe, and .bat counterparts?

It feels like a cleaner change, but having not gone through this document myself, I'm unaware if it will add confusion.

Another possibility would be to prepend a (cpython-venv) to imply that the virtual env is activated, but that feels unnecessarily messy.

@hugovk
Copy link
Member

hugovk commented Nov 18, 2023

Yeah, I saw that "You can now..." line earlier and wasn't sure... I haven't actually used any of the instructions on this page, they might be outdated and need updating at some point, so let's not worry too much about them now and update them later, if needed.

So I suggest we leave those ./python X as is, and only tabify the make X ones?

@lancegoyke
Copy link
Contributor Author

So I suggest we leave those ./python X as is, and only tabify the make X ones?

Sounds good 👍

I ended up forking cpython so that I could test building the docs on my Windows machine because it felt like the command might be the same on all OSes. Verdict: the .\ is indeed very important, haha.

@hugovk
Copy link
Member

hugovk commented Nov 19, 2023

Hmm, now we have three "Unix" / "macOS" / "Windows" tabs at the top.

And further down two "Unix/macOS" / "Windows" tabs.

The auto-detection is fine on first load, and if for example you start with macOS, then select "Windows", they all flip over. Good so far.

But if you then select "macOS", it doesn't also flip to the "Unix/macOS" one. But I don't think it really matters, it's just another click.

Thanks!

@hugovk hugovk merged commit 40d6473 into python:main Nov 23, 2023
4 checks passed
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.

2 participants