-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
New environment terminal language #18435
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
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 a lot for contributing! OTOH I'm not convinced that this is an improvement - the placeholder is already pretty clearly just a placeholder that the user should modify.
@@ -89,13 +89,13 @@ use on a preexisting environment! | |||
|
|||
A new environment can be set up with :: | |||
|
|||
python3 -mvenv /path/to/devel/env | |||
python3 -mvenv <file folder location> |
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 /path/to/devel/env
is pretty clearly a place holder?
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 don't think so since it's linux specific & I've seen it trip up enough users that I switched to in all my tutorials and the like.
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.
What is linux-specific? The forward slashes?
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.
to be clearer, I've seen folks not recognize it as a placeholder and literally type in /path/to/dev/env
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.
ok? And then it doesn't work because /path
doesn't exist (any more than <file folder location>
does) and they realize their mistake, right?
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'm pretty sure path completion works on both kinds of slashes in both cmd.exe and powershell.
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.
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.
Turns out my memory failed me :/
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 agree with @jeromefv and @story645 that we should not unnecessarily assume user knowledge here. IMHO a battle of /path/to/devel/env
vs. <file folder location>
is somewhat futile. It's an improvement, but marginal compared to the overall issue. The whole section (probably page) assumes too much and lacks motivation, context and explanation.
IMHO it should be something like
Installing Matplotlib for development
-------------------------------------
To decouple Matplotlib development from your working python environment, we
strongly recommend using a dedicated `virtual environment`_ (or alternatively
conda environment).
The virtual environment information is stored in a folder of your choice,
called `<dev env folder>` in the following`. A new environment can be set up
with ::
python3 -mvenv <dev env folder>
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.
You didn't actually follow @timhoffm 's suggestion to explain what <file folder location>
is supposed to mean.
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.
Accidental comment (I blame the app). :(
I'd say if someone is blindly typing:
the second error is at least pretty clear what 8000 the issue is. I'll note again that these are developer docs. I think we are allowed to assume some level of expertise in folks who want to set up a development environment. |
on windows, the latter doesn't even flag as an error: C:\Users\story\Projects\proposal>python3 -mvenv <file folder location>
The syntax of the command is incorrect.
C:\Users\story\Projects\proposal>python3 -mvenv /path/to/dev/env
Documentation contributions require setting up a development environment and in particular for some of the docs we want low barriers to entry for contributing. Maybe the solution will end up being that they use github codeblocks but I don't think that's camera ready yet. |
Why do you need an editable install to edit the documentation? |
because the documentation is in sphinx/rst with enough matplotlib custom stuff that github's preview doesn't handle it, so once you're doing anything on the scope of narrative docs you're gonna want to be able to build them. |
Hah, well if you want to build the docs locally, you've got a long haul after you manage to set up your dev environment. I never build locally, I just let CI do it for me. |
This was an error I ran into. It's not obvious to me that the file folder location is something that can change without any other context. As someone who is not a developer, I assumed that I had to gain write access to that specific file folder location. Upon researching that option, it was a very involved solution for a task. I only later independently discovered that I could change the file folder location and move forward in the process of building documentation. Searching for solutions for why I wasn't able to get write access only took me further away from the goal of creating a virtual environment in Python. I eventually solved my own problem; however, it would have been remedied by a simple language addition and an explanation for it as well. I don't see any conversely catastrophic harm done to the process when incorporating friendlier language aside from cohesion with Python developer docs and possible conflict using the direct copy/paste of terminal commands. |
Again, I think this is not an improvement. But other devs can dismiss my review. |
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.
On a more general note, we need to address both, the experienced user who just needs a brief summary, but also the novice who needs a lot more explanation.
Structuring is a key for serving both.
We should reduce long prose sections. One could e.g. summarize the steps at the top of the section with an item list (as an overview and for the experienced user) and/or add subsections explaining the steps in detail. Or use collapsible blocks for more context information (has anybody written a sphinx plugin for this?).
@@ -89,13 +89,13 @@ use on a preexisting environment! | |||
|
|||
A new environment can be set up with :: | |||
|
|||
python3 -mvenv /path/to/devel/env | |||
python3 -mvenv <file folder location> |
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 agree with @jeromefv and @story645 that we should not unnecessarily assume user knowledge here. IMHO a battle of /path/to/devel/env
vs. <file folder location>
is somewhat futile. It's an improvement, but marginal compared to the overall issue. The whole section (probably page) assumes too much and lacks motivation, context and explanation.
IMHO it should be something like
Installing Matplotlib for development
-------------------------------------
To decouple Matplotlib development from your working python environment, we
strongly recommend using a dedicated `virtual environment`_ (or alternatively
conda environment).
The virtual environment information is stored in a folder of your choice,
called `<dev env folder>` in the following`. A new environment can be set up
with ::
python3 -mvenv <dev env folder>
Jody do you have any particular issues getting the docs to build? I think that with perfect documentation, an experienced Python developer would be able to set up our docs to build locally in, say 5min (less time than it takes for Sphinx to actually run :/). I know that right now we're missing documentation of some particular dependencies, but besides those system-specific brew/apt-get/whatever commands, ideally a
Obviously there is quite a bit of disagreement among the core team as to what level of expertise should be expected at different places in the docs. As someone who teaches people at many different levels of Python expertise, I would argue that this PR is an improvement as-is, but would recommend against any larger efforts to solve the problem of making docs contributions easier by making setting up a dev environment easier.
Sphinx is fundamentally tricky. RST is fundamentally tricky. I don't think that we should bend over backwards for the microscopic chance that we entice a novice developer to spontaneously start making large enough changes that they really need to see how they render before sending in a PR. And for simpler stuff, instead of funneling anyone that wants to make a doc contribution through setting up the whole dev environment, we should just have them send it into us. I for one don't think modifying a well-meaning docs PR with bad RST syntax to be compliant is really much harder than reviewing it in the first place. To me, the long term solution can be found with our migration to a new Sphinx theme. As we start to set it up, having (for example) a floating element that says "Found something confusing? Click here to fix it!" that sends them to a page with the instructions for editing that specific page ((1) fork, (2) clone, (3) which file to edit, pref with line numbers, (4) simple add/commit/push, (5) click PR button). |
I think the real real solution here is suggest they use GitHub codespaces given the environment builds there. Especially since we have an I think working docker build script for the docs that can probably be configured into a GitHub action. I agree w/ @timhoffm though that the real issue here is the question of audience & that expectation setting at the top of the doc would be useful. |
OT, but, I don't build the docs locally because the dependencies are non trivial, not all listed, and a messy guessing game of |
"They're dev docs, they're allowed to be complicated" as a general sentiment is a fantastic method to ensure we don't on-board any novice developers. I'm not saying we have to bend over backwards. In this particular case, where we have a demonstrated case that the current content was confusing and we try to clarify in a way that requires minimum additional prose, I'm 👍 .
@story645 I don't think you want to rely on codespaces, as here's what they say about pricing:
|
@jeromefv the only thing that's holding up approval from me is the slash order for the windows path. |
Not too sure you did that rebase right. |
Co-authored-by: Jody Klymak <jklymak@gmail.com>
df0b3db
to
1c22fe8
Compare
@jklymak @story645 @brunobeltran I believe with Bruno's help that I've resolved these issues! Thank you to everyone for helping me get through my first PR successfully. I appreciate all you've done! |
Doc build failure is unrelated to this PR (c.f. #18492). |
@jeromefv Congratulations on your first contribution to Matplotlib! 🎉 |
PR Summary
Modified terminal command language to indicate that user can control directory for creating new environment.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
andpydocstyle<4
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).