adding saved replies to development guide, under triage #23109
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
| Pull Requests | ||
| ------------- | ||
|
|
||
| First Pull Request Merged |
There was a problem hiding this comment.
Just thinking: Could we have a bot write that message or is that too impersonal? We already have an automated welcome message (as you can see in this PR 😄).
There was a problem hiding this comment.
I think I would rather do them myself while I can manage the load. I would need to set up getting notifications for new PRs being merged. But I will consider it if it becomes unmanageable
There was a problem hiding this comment.
I have a slight preference for a human doing this. We do not have such a huge influx of new contributors and I think it does help build the inter-personal relationship.
|
Is the ideas to use these with "Saved Replies" or some such? https://docs.github.com/en/get-started/writing-on-github/working-with-saved-replies/about-saved-replies. Can we add these as a project? |
My understand of the GH ui is no (if you look in the notes there is a link to a feature request to 👍🏻 ). |
There was a problem hiding this comment.
I think the explanation at the start could have been a single four sentence paragraph. I took a first pass at trimming, but could probably do more.
Similarly, the actual canned responses are quite long, and suffer the potential fate of drifting out of date with the actual docs. Suggest we link the actual docs.
doc/devel/saved_replies.rst
Outdated
| To facilitate DRY (do not repeat yourself) communication principles, reduce potential bias and errors in | ||
| communication, and consistently maintain respectful and welcoming communication we decided to develop and collect a set | ||
| of reusable replies for recurring project needs. |
There was a problem hiding this comment.
| To facilitate DRY (do not repeat yourself) communication principles, reduce potential bias and errors in | |
| communication, and consistently maintain respectful and welcoming communication we decided to develop and collect a set | |
| of reusable replies for recurring project needs. | |
| To avoid repetition, reduce potential bias and errors, and maintain respectful and welcoming communication, Matplotlib has a set | |
| of reusable replies for common issues and pull request feedback. |
doc/devel/saved_replies.rst
Outdated
| $ git rebase --interactive HEAD~11 | ||
|
|
||
|
|
||
| And follow the instructions, basically replace 'pick' by 'f' in all but the first commit. Then update your main branch |
There was a problem hiding this comment.
| And follow the instructions, basically replace 'pick' by 'f' in all but the first commit. Then update your main branch | |
| and follow the instructions, basically replace 'pick' by 'fixup' (or simply 'f') in all but the first commit. Then update your main branch |
|
Is the idea that these should be copied and pasted from the page? It will be a bit of a hassle with formatting parts of it. I think that it would be beneficial to provide these in "raw" Github Markdown so that it is easy to add them to "Default replies" (which seems to be connected to a user). In that way one can easily add them and have them ready. ( https://docs.github.com/en/get-started/writing-on-github/working-with-saved-replies ) |
|
My idea was to provide the markdown actually, but then I'm not sure what's the best way to include it. So far my ideas to include it were:
Would be happy to get guidance here, as to what would be most convenient. |
|
I don't know about copying, but you can put the markdown in literals. |
|
Yeah sphinx will let you embed raw files (it's what we use on the citation page) https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-literalinclude https://github.com/matplotlib/matplotlib/blob/main/doc/users/project/citing.rst |
|
Thanks everyone for all the feedback so far! |
|
@noatamir I've marked this as draft. Please mark as ready-for-review when you are done with the revision. |
|
@noatamir we generally prefer to rebase PRs on master rather than merging master in PRs. This tends to result in simpler PRs. Not a difference here because there were no conflicts, but I suggest to grow the habit. A rebased PR is essentially the same as if you had made the PR st a later point in time. That drops any complexity you'd explicitly keep in a merge. |
There was a problem hiding this comment.
I added some safety catches to the git workflows section. Folks should always create a backup branch unless they know what they are doing, because most people will push to origin before they realize their mistake, and then their only recourse to fix bad commits is if they saved a backup as part of their backup system.
| Git opens the last commits you made in our terminal editor (often it's vim) and you need to follow the instructions in | ||
| the file. Basically replace 'pick' by 'fixup' (or simply 'f') in all but the first commit (exit vim using `:wq` to | ||
| write/save and quite). Then update your main branch from upstream, change back to the PR branch and do:: |
There was a problem hiding this comment.
| Git opens the last commits you made in our terminal editor (often it's vim) and you need to follow the instructions in | |
| the file. Basically replace 'pick' by 'fixup' (or simply 'f') in all but the first commit (exit vim using `:wq` to | |
| write/save and quite). Then update your main branch from upstream, change back to the PR branch and do:: | |
| Git opens the last commits you made in your editor (see `echo $EDITOR`) and you need to follow the instructions in the file. Basically replace 'pick' by 'fixup' in all but the first commit. When done, exit your editor. | |
| Next, update your main branch from upstream:: |
There was a problem hiding this comment.
I think you should be editor agnostic here, and rather refer to the environment variable that controls this behaviour in git.
There was a problem hiding this comment.
I thought adding the exit vim clue would save a lot of people a lot of searching 😉 I saw something similar in the GitLab documentation for interactive rebase and I thought it was kind and helpful. I tried to keep it more concise and be clear it's just an example.
There was a problem hiding this comment.
I'd hate for people to read our instructions and come away with the impression that they have to learn vi before they can master a rebase. If you want a generic vt100-compatible text editor pico/nano would be a lot more suitable for beginners who are just slavishly following our instructions.
There was a problem hiding this comment.
I wholeheartedly agree. I don't think I implied that people need to master vim though?! At least on macs vim is the default editor, and I don't know how many beginners change it 🤷♀️ So likely the first time they run a rebase it would open in vim, and they might not know what to do. This means we need to add instruction for mac users to change the .bash_profile before they run rebase to nano to change their default, and/or a hint on how to exit vim. I am not sure what's the common default editor on windows and linux.
| $ git push --force-with-lease origin HEAD | ||
|
|
||
| If you have any problems, feel free to ask questions. | ||
|
|
||
| PS. | ||
| If at any point anything goes wrong, and you don't know what to do, just do:: | ||
|
|
||
| $ git rebase --abort | ||
|
|
||
| and everything will go back to the way it was in the before ``$ git rebase`` times, and you can come back to your PR, | ||
| or to `gitter`_, and ask us for help, or that we do the rebase for you 😉. |
There was a problem hiding this comment.
| $ git push --force-with-lease origin HEAD | |
| If you have any problems, feel free to ask questions. | |
| PS. | |
| If at any point anything goes wrong, and you don't know what to do, just do:: | |
| $ git rebase --abort | |
| and everything will go back to the way it was in the before ``$ git rebase`` times, and you can come back to your PR, | |
| or to `gitter`_, and ask us for help, or that we do the rebase for you 😉. | |
| If at any point anything goes wrong, and you don't know what to do, just do:: | |
| $ git rebase --abort | |
| and everything will go back to the way it was in the before ``$ git rebase``, and you can come back to your PR, | |
| or to `gitter`_, and ask us for help, or that we do the rebase for you 😉. | |
| If you finish the rebase, but it is _still_ wrong, you can still get back to your backup branch, as noted above. | |
| When you think your PR has the right commits (look at `git log`) based on the latest upstream/main you can push to GitHub:: | |
| $ git push --force-with-lease origin my-working-branch | |
There was a problem hiding this comment.
I like the addition of the backup! 🙇♀️ I used a backup tag so far to revert. Probably the branch strategy is less stressful and I should use that too 😃
| If you want to rebase, the first thing to do is to squash all your commits into one, which will make the job easier. | ||
| Make sure you are in the PR branch, then to rebase do:: | ||
|
|
There was a problem hiding this comment.
| If you want to rebase, the first thing to do is to squash all your commits into one, which will make the job easier. | |
| Make sure you are in the PR branch, then to rebase do:: | |
| If you want to rebase, the first thing to do is to make a backup branch:: | |
| $ git checkout my-working-branch | |
| $ git checkout -b backup-my-working-branch | |
| $ checkout my-working-branch | |
| If you mess anything up, you can always get back to where you were pre-rebase by doing:: | |
| $ git checkout my-working-branch | |
| $ git reset --hard backup-my-working-branch | |
| Next, we suggest you squash all your commits into one, which will make the rebase easier. | |
| Make sure you are in the PR branch, then to rebase do:: | |
| $ git rebase main | ||
|
|
There was a problem hiding this comment.
| $ git rebase main | |
| $ git fetch upstream | |
| $ git rebase upstream/main | |
|
Note, you are probably going to slow this down by including the git workflows here. I wonder if that would be better served as a different PR? |
|
@jklymak I included the git workflows based on Tim's suggestion to make the rebase saved reply short, and add a link to the full rebase instructions in the docs. I would like to add more saved replies after we are done with this PR, but the rebase saved reply is the top priority for me, and I'd like to know what's needed to get that in. |
|
I agree with linking out, but I'm just suggesting two PRs - the saved replies can point to the existing instructions, and the other PR can replace the existing instructions. I think its confusing to have git instructions in two places as proposed here. Note that the existing instructions are https://matplotlib.org/stable/devel/gitwash/index.html. Replacing them comprehensively is a great idea, and what you have is a reasonable start to one of the hardest steps. |
I think we sort of all agree that the gitwash instructions are kinda terrible though, so why not as a first pass leave this as is w/ the git instructions and then as a follow on Noa (or someone else) can improve the instructions? that way the replies won't have to be updated when the follow on PR goes up. Kinda like how we have the 'install releases' and 'install for dev' pages, this is the 'quick git guide for fixing problems' and the existing gitwash is the longer 'how to github' (which is wrong anyway, so maybe soft deprecate?) |
This PR Addresses discussions raised in the Matplotlib Community Meeting see:
https://hackmd.io/jd_7FjxNQ4y7XgNknvmvGQ#May-12-2022,
and https://hackmd.io/jd_7FjxNQ4y7XgNknvmvGQ#Canned-response-for-needing-a-rebase.
The goal was to create a space in the developer guide to share saved replies the community can share and contribute to.
In addition, I wanted to create an introduction to why we are writing saved replies, and how our community can benefit from them. So that the shared concepts and aspirations discussed in the community calls are documented as well.
After the introduction, I included 2 saved replies, both pertaining to PRs:
I think it fits well under triage for now, but I also didn't want to spend too much time on structure considering a re-structure is on my to-do list.
The docs built locally, so I hope it will also pass the CI 🤞
I look forward to your feedback 🙇♀️