-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix #10508 (modernize virtualenv doc) #11367
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
Conversation
Fixed undefined label. |
I am changing the milestone to 4.0.x given this doc is very old, but if backport is a pain, release manager can change the milestone (again). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, nice clean-up! Some comments below.
I'll also ping someone who I think does not use conda
to review, just to have extra pairs of eyes.
This is rebased now on top of #11622, so it depends on that first. But otherwise this is ready to go. |
This comment has been minimized.
This comment has been minimized.
#11622 is merged but there is still a conflict. I could resolve it for you but I am afraid it would mess up the commit authorship. Given this is milestoned to a bugfix release, it doesn't have to go in by 4.3 feature freeze. It just won't end up in 4.3 but will be in a later bugfix release, that's all. I'll leave it to follow-up when you can, unless you state otherwise. Thanks! |
Rebased. No problem about missing the freeze. Sorry I haven't been available lately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final nitpicks.
@@ -1,105 +0,0 @@ | |||
:orphan: | |||
|
|||
.. _using-virtualenv: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this ref might break some docs downstream. Example report along similar vein: #11754
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. A quick search turned up at least one project that references that page: https://pypi.org/project/pywicta/0.1.dev19/
Good opportunity to try out sphinx-redirects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a redirect using sphinx-reredirects (I kept missing the "rere" in the name before).
@pllim Since this adds a new doc build dependency do you think I should make a separate PR for that first, or just go ahead and do it here since this is where the need arose?
setup.cfg
Outdated
@@ -89,6 +89,7 @@ docs = | |||
scipy>=1.1 | |||
matplotlib>=3.1,!=3.4.0 | |||
sphinx-changelog>=1.1.0 | |||
sphinx-reredirects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this package is from https://gitlab.com/documatt/sphinx-reredirects ? Are we worried about single-point-of-failure here, in that the author will abandon this package and then we end up with a bunch of broken links?
I think it is okay to introduce a new dependency here instead of a new PR. But I want to make sure we are not using someone's random pet project that will get abandoned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a less controversial approach to let this PR get merged sooner than later is, as you said, introduce this dependency in a follow-up PR. In the meantime, instead of deleting the ref to docs/development/workflow/virtualenv_detail.rst
, move that ref somewhere in the new page, or keep the old page with the ref, but scrub out the content and point to the new page without this reredirect thingy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's pretty low-risk. The developer seems open to accepting patches, and the extension itself is about 100 lines so if it falls into disrepair, could easily be merged into sphinx-astropy or the like.
Otherwise I don't think this is worth expending any more effort on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see what others say.
I haven't seen anyone express a strong opinion on this yet, so for the sake of getting this done I'll revert the use of sphinx-reredirects for now and create a redirect in RTD before we merge this. Then I'll open a separate issue for adding sphinx-reredirects |
…d shift some details to the newer page. Add details on pipenv, as well as a link to the Hitchhiker's Guide to Python chapter on the subject, which was helpful for writing the pipenv docs. Other minor cleanup to that page. Added Pipfile[.lock] to .gitignore for now, since we don't currently include a Pipfile in the source, though it is created by default when installing a package with pipenv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can set up the manual redirect for using-virtualenv
, that is good enough for me. But I'll withhold approval until @mhvk is happy with the text. Thank you all for your patience!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is all good modulo my small grammatical comment. I'll approve pending that.
detail (I did not see a mention of the term "prefix" anywhere which is at least good for *NIX users to be exposed to) I admit I fudged the details on Windows a bit, but if anyone wants to expand on it, in short virtualenv on Windows essentially copies some of the contents of C:\PythonX.Y (if that's where you installed it originally), and IIRC also makes some changes by way of site_customize.py. But I have not used virtualenv on Windows in a long time so I'm hazy on the precise details.
Changes are all great, thanks! Is the manual redirect part of this, or separate? If separate, feel free to merge. |
Merging and setting up the manual redirect for now. |
I added the redirect but it doesn't seem to have taken effect yet. In the past I seem to recall it being pretty quick. I'll try again in a few minutes... |
OK, the redirect works now. |
Thanks! |
Remove old, outdated virtualenv documentation page and shift some details to the newer page.
Added details on pipenv, as well as a link to the Hitchhiker's Guide to Python chapter on the subject, which was helpful for writing the pipenv docs. Other minor cleanup to that page.
Added Pipfile[.lock] to .gitignore for now, since we don't currently include a Pipfile in the source, though it is created by default when installing a package with pipenv.
Fix #10508