Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@noatamir noatamir commented May 23, 2022

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:

A reply following a first PR merge to encourage further contributions and introduce all our communication channels, as well as the new contributor meeting
An offer of information and/or help with a needed rebase, which was already discussed in the community meeting.

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 🙇‍♀️


 adding saved replies to development guide, under triage

34fa4be

github-actions[bot]

github-actions bot reviewed

May 23, 2022

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

timhoffm

timhoffm reviewed

May 23, 2022

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.

Thanks @noatamir this is a great start!

doc/devel/saved_replies.rst Outdated Show resolved Hide resolved

doc/devel/saved_replies.rst

Pull Requests

-------------

First Pull Request Merged

Copy link

Member

@timhoffm timhoffm May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

@noatamir noatamir May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ☺️. It's between UX and building relationships, and feasibility.

Copy link

Member

@tacaswell tacaswell Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

doc/devel/saved_replies.rst Outdated Show resolved Hide resolved

Copy link

Member

jklymak commented May 23, 2022 •

edited

Loading

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?

@tacaswell

Copy link

Member

tacaswell commented May 23, 2022

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

jklymak

jklymak reviewed

May 24, 2022

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.

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 Show resolved Hide resolved

doc/devel/saved_replies.rst Outdated

Comment on lines 7 to 9

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.

Copy link

Member

@jklymak jklymak May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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 Show resolved Hide resolved

oscargus

oscargus reviewed

May 25, 2022

( https://docs.github.com/en/get-started/writing-on-github/working-with-saved-replies )

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

Copy link

Member

@oscargus oscargus May 25, 2022 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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

@oscargus

Copy link

Member

oscargus commented May 25, 2022

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.

Copy link

Contributor Author

noatamir commented May 27, 2022

My idea was to provide the markdown actually, but then I'm not sure what's the best way to include it.
Do you have suggestions?

So far my ideas to include it were:

In the docs, which seemed repetitive at first, but maybe I can use some copying functionality of sphinx which I haven't explored yet fully. I can look into it.
In GitHub Wiki, and link to that in the Docs. That way you don't need to go to the Docs to "go get them"

Would be happy to get guidance here, as to what would be most convenient.

https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-literalinclude

Copy link

Member

jklymak commented May 27, 2022

I don't know about copying, but you can put the markdown in literals.

@story645

Copy link

Member

story645 commented May 27, 2022 •

edited

Loading

Yeah sphinx will let you embed raw files (it's what we use on the citation page)

https://github.com/matplotlib/matplotlib/blob/main/doc/users/project/citing.rst

Copy link

Contributor Author

noatamir commented May 27, 2022

Thanks everyone for all the feedback so far!
I'll make some revisions and we'll see if we're closer to something we can add soon ^_^

@story645 story645 added the Documentation label

May 30, 2022

@timhoffm timhoffm marked this pull request as draft

June 15, 2022 15:58

@timhoffm

Copy link

Member

timhoffm commented Jun 15, 2022

@noatamir I've marked this as draft. Please mark as ready-for-review when you are done with the revision.


 Merge branch 'matplotlib:main' into adding-saved-replies

d9e15be

@timhoffm

Copy link

Member

timhoffm commented Jun 18, 2022 •

edited

Loading

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

noatamir added 2 commits

June 30, 2022 18:27


 adding saved replies and git workflows to developer guide, under triage.


 resolved merge, will rebase and clean

eae47f3

Copy link

Contributor Author

noatamir commented Jun 30, 2022

👋 @timhoffm, @jklymak et. al. I did a whole rewrite based on your feedback and suggestions.
I'd appreciate if you can give this another look sometime, and let me know what still needs work 🙇‍♀️

noatamir

noatamir commented

Jun 30, 2022

doc/devel/triage.rst Outdated Show resolved Hide resolved


 oops.

fabd529

jklymak

jklymak reviewed

Jul 1, 2022

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.

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.

Comment on lines +20 to +22

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

Copy link

Member

@jklymak jklymak Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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

Copy link

Member

@jklymak jklymak Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be editor agnostic here, and rather refer to the environment variable that controls this behaviour in git.

Copy link

Contributor Author

@noatamir noatamir Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Member

@jklymak jklymak Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@noatamir noatamir Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +32 to +42

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

Copy link

Member

@jklymak jklymak Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

$ 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

Copy link

Contributor Author

@noatamir noatamir Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😃

Comment on lines +14 to +16

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

Copy link

Member

@jklymak jklymak Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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

Comment on lines +24 to +25

$ git rebase main

Copy link

Member

@jklymak jklymak Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

$ git rebase main

$ git fetch upstream

$ git rebase upstream/main

Copy link

Member

jklymak commented Jul 1, 2022

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?

Copy link

Contributor Author

noatamir commented Jul 1, 2022

@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.
If you think the name of the page or its location could be improved, I'd be happy to get input!
But without this page, I would need to remove the rebase saved reply, which is the original reason for this PR 😕

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.