-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: Add documentation on codespaces usage #26201
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
The tab on develop looks good. Why is the information then duplicated on a second page? |
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 swear I like this more than my review makes it seem like I do :/
It's one of these things where I think it's great as a standalone guide but since we have all these other docs would prefer if it tied into them more.
Because the |
Is that a usual thing for codespaces? Seems strange to pop open an editable file that is part of the repo. Regardless id just have the markdown file have a link to the canonical instructions and not repeat info. |
doc/devel/codespaces.md
Outdated
`mpl-dev`: | ||
|
||
``` | ||
conda activate mpl-dev |
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 there is a vscode setting file we can drop to tell vscode we want to use the environment and it will auto-activate for us.
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 will investigate but this was not possible until recently because of the way the terminal is fired up when codespaces is first opened. They have been making a lot of updates lately so I'll verify.
I'm less worried about the duplicate text between the tabs. I think reading a unified top-to-bottom flow is nicer than reading it broken up. Is there anything we can do about the navigation on the left? I am not sure what the right order is, but having "Contribute" below the MEP docs and near the bottom seems less than ideal. Looking at this whole section I think the bigger problem is less the repetition, but more the disjointedness as you read through the sections in the order on the left....
If the goal is to make it easy for people to get started, I'm in favor of it being as obviously self documenting in-line as possible. |
doc/devel/codespaces.md
Outdated
in-browser development environment, that comes with the appropriated setup to | ||
contribute to Matplotlib). | ||
|
||
A brief overview of the workflows is as follows. |
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 know the purpose of this document may not yet be determined. But, if this is ultimately going to be a beginner's guide for working in codespace, I suggest we remove the workflow for the preferred method throughout and just mention it with a link once. Afterward, only document for the codespaces workflow.
Then change Line 24 to:
A brief overview of the workflow with Codespaces follows.
* Reducing duplication by linking out to contribute guide * Rewording instructions and adding notes on codespaces-specific workflows
Here's a second take. I made the codespaces.md file simpler to just link out to the actual contributing guide and reorganized the build instructions a little bit. I think I've addressed most comments but happy to have more feedback. Cheers! Relevant artifact: https://output.circle-artifacts.com/output/job/b7e3320f-aab1-4533-8fd7-98b239eaf25f/artifacts/0/doc/build/html/devel/contribute.html#contributing-code |
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 style nits but generally I think this is awesome and addressed the concerns!
Co-authored-by: marbled-toast <69227427+marbled-toast@users.noreply.github.com> Co-authored-by: hannah <story645@gmail.com>
8f9a882
to
8c3a9c0
Compare
Here's another take: https://output.circle-artifacts.com/output/job/279bd963-85a5-45e1-89a6-d8bc05a913d5/artifacts/0/doc/build/html/devel/contribute.html#contributing-code The codespaces.md file is now just a list of links, and all the docs are in the Contribute page. I think this is much simpler and better! @story645 the nested numbering is a bit tricky, let me know what you think of this version. |
I see what you mean. Bullets or even spaces - so it's 3 short paragraphs- is also fine. I think it'd be better for the three steps in code spaces are clearly visually separated but I don't care much how. And yeah I also like this version much much better. |
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.
just posted an example of what I mean by visually distinguishable if the double listing takes up too much space.
@@ -0,0 +1,9 @@ | ||
# Contributing to Matplotlib using GitHub codespaces |
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 really 😻 the sign-posting here
@meeseeksdev backport to v3.8.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
I don't feel like doing that backport close to 3.8 release. |
…201-on-v3.8.x Backport PR #26201 on branch v3.8.x (DOC: Add documentation on codespaces usage)
PR summary
This PR adds a skeleton for codespaces usage documentation. There are definitely improvements to be made but I have a couple of points:
Addresses #26169
PR checklist