Skip to content

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

Merged
merged 6 commits into from
Jun 21, 2021
Merged

Fix #10508 (modernize virtualenv doc) #11367

merged 6 commits into from
Jun 21, 2021

Conversation

embray
Copy link
Member

@embray embray commented Mar 3, 2021

  • 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

@embray embray added Docs Affects-dev PRs and issues that do not impact an existing Astropy release labels Mar 3, 2021
@embray embray added this to the v4.3 milestone Mar 3, 2021
@embray embray requested a review from pllim March 3, 2021 22:41
@embray
Copy link
Member Author

embray commented Mar 4, 2021

Fixed undefined label.

@pllim pllim changed the title Fix #10508 Fix #10508 (modernize virtualenv doc) Mar 5, 2021
@pllim pllim modified the milestones: v4.3, v4.0.5 Mar 5, 2021
@pllim pllim added no-changelog-entry-needed and removed Affects-dev PRs and issues that do not impact an existing Astropy release labels Mar 5, 2021
@pllim
Copy link
Member

pllim commented Mar 5, 2021

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).

Copy link
Member

@pllim pllim left a 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.

@pllim pllim requested a review from saimn March 5, 2021 20:05
@eteq eteq modified the milestones: v4.0.5, v4.0.6 Mar 18, 2021
@embray
Copy link
Member Author

embray commented Apr 22, 2021

This is rebased now on top of #11622, so it depends on that first. But otherwise this is ready to go.

@pllim

This comment has been minimized.

@pllim
Copy link
Member

pllim commented Apr 30, 2021

#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!

@embray
Copy link
Member Author

embray commented May 17, 2021

Rebased. No problem about missing the freeze. Sorry I haven't been available lately.

Copy link
Member

@pllim pllim left a 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:
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@embray
Copy link
Member Author

embray commented Jun 14, 2021

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.
@embray
Copy link
Member Author

embray commented Jun 14, 2021

I opened #11837 for the redirects stuff. @pllim if it's ok with you I'll go ahead and merge this and set up a manual redirect for now?

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Very small comment

Copy link
Member

@pllim pllim left a 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!

Copy link
Contributor

@mhvk mhvk left a 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.
@mhvk
Copy link
Contributor

mhvk commented Jun 16, 2021

Changes are all great, thanks! Is the manual redirect part of this, or separate? If separate, feel free to merge.

@embray
Copy link
Member Author

embray commented Jun 21, 2021

Merging and setting up the manual redirect for now.

@embray embray merged commit f911ec1 into astropy:main Jun 21, 2021
@embray embray deleted the issue-10508 branch June 21, 2021 08:39
@embray
Copy link
Member Author

embray commented Jun 21, 2021

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...

@embray
Copy link
Member Author

embray commented Jun 21, 2021

OK, the redirect works now.

@pllim
Copy link
Member

pllim commented Jun 21, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modernize documentation on how to use virtualenv
4 participants