Conversation
|
Tagging @noatamir for visibility ;) |
|
I think the CI failure is unrelated. |
|
I think that was fixed. Can you rebase? |
|
We have a redirect-from directive that should probably be used here so the old links don't just 404? Though I think there is now a public extension that does the same thing that maybe we should be using? |
|
For some reason |
|
Rebased, fingers crossed! |
|
Where should we re-direct the gitwash pages to? I can see cases for an oldversion of gitwash in our docs, a cannonical version of gitwash (if that exists?), or all at "development workflow". |
|
I would just redirect them all to our new docs versus gitwash which I do not think has been updated for ever. That would also be better seo wise |
|
Yeah we've been discussing for a while a set of version independent docs & that the development docs (I think git instructions included) are part of those? |
|
If you need specific anchors for the redirects I'm happy to add those too - then we can just repurpose the dev instructions and not rely on external pages. |
story645
left a comment
story645
left a comment
There was a problem hiding this comment.
the restructuring all seems reasonable to me and this seems like a good first step in migrating away from the gitwash.
|
On a bit of consideration we should have all of the old gitwash pages re-direct to the moved development_workflow page. I left a few minor wording comments, but other wise 👍🏻 |
doc/devel/coding_guide.rst
Outdated
| .. __: https://github.com/psf/black/issues/1984 | ||
|
|
||
| * Each high-level plotting function should have a simple example in the | ||
| ``Example`` section of the docstring. This should be as simple as possible |
There was a problem hiding this comment.
I suggest we drop this. We don't consistently have this in our current docs.
Examples calls are usually not very instuctive without seeing the visual output. And we have the sphinx-gallery mini galleries at the bottom of the docstring.
We don't consistently have this in our current docs, and I
If we want this, we should move it to https://matplotlib.org/stable/devel/documenting_mpl.html#writing-docstrings
There was a problem hiding this comment.
I think you're right - and this is actually pretty well explained already in that page. Let me know if what I wrote makes sense, or if we should remove entirely.
doc/devel/development_setup.rst
Outdated
| current working directory, and tell git all your pull requests should target the | ||
| ``upstream`` repository. |
There was a problem hiding this comment.
I think this is not correct. The PR target is defined by the repo you forked from. Whether you have an upstream connection does not influence the (default) PR target.
I suggest to just leave off the second sentence. - We maybe don't need to explain every command in detail. Otherwise, you'd have to explain here the full triangular relation between upstream, origin and local (https://www.tomasbeuzen.com/post/git-fork-branch-pull/).
There was a problem hiding this comment.
How about
This will place the sources in a directory :file:matplotlib below your current working directory, and set up the upstream remote to point to the Matplotlib main repository.
There was a problem hiding this comment.
Oh nevermind, I had to rebase and saw this was actually ok - no need to mention upstream here at all.
There was a problem hiding this comment.
I am a bit lost as to the state of things, but do like
This will place the sources in a directory :file:matplotlib below your current working directory, and set up the upstream remote to point to the Matplotlib main repository.
I think the source of the problems are that the remote names are not actually semantic, but a bunch of tools have made (incompatible) assumptions about the meanings of names.
There was a problem hiding this comment.
I added a github documentation link, let me know if it helps clarify the instructions.
|
Would it be possible to work in links to https://tacaswell.github.io/think-like-git.html and https://tom.preston-werner.com/2009/05/19/the-git-parable.html ? |
Sure - I had to rebase to fix some conflicts and will review all the links and see what I can do 👍 |
|
Thanks @timhoffm - I was explicitly not doing any content changes (just making sure nothing was lost) to make this one easier to review, but I agree with your points. I'll get to them as soon as possible 👍🏻 |
|
@melissawm sorry I did not read/rembember your original comment. If you want to tackle changes separately, we can also merge this and continue from there. |
melissawm
force-pushed
the
fix-gitwash
branch
from
cabfb97 to
42aa669
Compare
melissawm
force-pushed
the
fix-gitwash
branch
from
42aa669 to
e592e7a
Compare
|
Hi folks - so sorry for the long delay here! I've rebased and believe I caught all of the comments, let me know if there are any other changes to be made. Cheers! |
|
I think the re-directs are still missing, but I'll open a follow on PR to add those. |
|
Thank you @melissawm ! |
6 participants
PR Summary
This PR moves some of the gitwash content to the main body of the contributor guide.
I deliberately chose not to make any huge content changes to make it easier to review, but there's a series of follow-ups I would be interested in doing (either here or on other PRs) such as rewording and reorganizing pages when appropriate.
Feedback is welcome!
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).