8000 New environment terminal language by jeromefv · Pull Request #18435 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 1 commit into from
Sep 15, 2020
Merged

Conversation

jeromefv
Copy link
Contributor
@jeromefv jeromefv commented Sep 8, 2020

PR Summary

Modified terminal command language to indicate that user can control directory for creating new environment.

PR Checklist

  • [ x] Has pytest style unit tests (and pytest passes).
  • [ x] Is Flake 8 compliant (run flake8 on changed files to check).
  • [ x] New features are documented, with examples if plot related.
  • [ x] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [x ] Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • [x ] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [x ] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jeromefv jeromefv changed the title Practice branch New environment terminal language Sep 8, 2020
jklymak
jklymak previously requested changes Sep 8, 2020
Copy link
Member
@jklymak jklymak left a 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>
Copy link
Member

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?

Copy link
Member
@story645 story645 Sep 8, 2020

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.

Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member
@story645 story645 Sep 8, 2020

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.

I just tested it...
ezgif-7-a965b63e1dd2

Copy link
Contributor

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 :/

Copy link
Member
@timhoffm timhoffm Sep 10, 2020

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>

Copy link
Member

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.

Copy link
Member
@story645 story645 left a 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). :(

@jklymak
Copy link
Member
jklymak commented Sep 9, 2020

I'd say if someone is blindly typing:

$ python3 -mvenv <file folder location>
zsh: parse error near `\n'
$
$ python3 -mvenv /path/to/dev/env
Error: [Errno 30] Read-only file system: '/path'

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.

@tacaswell tacaswell added this to the v3.4.0 milestone Sep 9, 2020
@story645
Copy link
Member
story645 commented Sep 9, 2020

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

I think we are allowed to assume some level of expertise in folks who want to set up a development environment.

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.

@jklymak
Copy link
Member
jklymak commented Sep 9, 2020

Documentation contributions require setting up a development environment and in particular for some of the docs we want low barriers to entry for contributing.

Why do you need an editable install to edit the documentation?

@story645
Copy link
Member
story645 commented Sep 9, 2020

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.

@jklymak
Copy link
Member
jklymak commented Sep 9, 2020

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.

@jeromefv
Copy link
Contributor Author
jeromefv commented Sep 9, 2020

I'd say if someone is blindly typing:

$ python3 -mvenv <file folder location>
zsh: parse error near `\n'
$
$ python3 -mvenv /path/to/dev/env
Error: [Errno 30] Read-only file system: '/path'

the second error is at least pretty clear what 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.

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.

@jklymak
Copy link
Member
jklymak commented Sep 9, 2020

Again, I think this is not an improvement. But other devs can dismiss my review.

Copy link
Member
@timhoffm timhoffm left a 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>
Copy link
Member
@timhoffm timhoffm Sep 10, 2020

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>

@brunobeltran
Copy link
Contributor

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.

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 git clone, followed by an appropriate pip install -r and a make html should suffice.

I think we are allowed to assume some level of expertise in folks who want to set up a development environment.

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.

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.

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

@story645
Copy link
Member

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

@jklymak
Copy link
Member
jklymak commented Sep 11, 2020

OT, but, I don't build the docs locally because the dependencies are non trivial, not all listed, and a messy guessing game of brew and conda on my machine. Definitely I've done it, but I really don't see the point - it takes forever to build anyways, so why not just let another machine somewhere do it? Well, other than that is probably bad for the environment.

@jklymak jklymak dismissed their stale review September 11, 2020 00:54

not consensus

@dopplershift
Copy link
Contributor

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

I think the real real solution here is suggest they use GitHub codespaces given the environment builds there.

@story645 I don't think you want to rely on codespaces, as here's what they say about pricing:

Pricing for Codespaces has not yet been finalized. Code-editing functionality in GitHub will always be free. We plan to offer simple pay-as-you-go pricing for Codespaces cloud environments. Codespaces is free during the limited beta.

@story645
Copy link
Member

@jeromefv the only thing that's holding up approval from me is the slash order for the windows path.

@QuLogic
Copy link
Member
QuLogic commented Sep 15, 2020

Not too sure you did that rebase right.

Co-authored-by: Jody Klymak <jklymak@gmail.com>
@jeromefv
Copy link
Contributor Author

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

@timhoffm
Copy link
Member

Doc build failure is unrelated to this PR (c.f. #18492).

@timhoffm timhoffm merged commit 5488ba5 into matplotlib:master Sep 15, 2020
@timhoffm
Copy link
Member

@jeromefv Congratulations on your first contribution to Matplotlib! 🎉

@jeromefv jeromefv deleted the practice_branch branch September 15, 2020 20:15
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.

9 participants
0