Skip to content
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

Enable Squash and Merge on every repo and document merge procedure #131

Open
hug-dev opened this issue Sep 21, 2021 · 3 comments
Open

Enable Squash and Merge on every repo and document merge procedure #131

hug-dev opened this issue Sep 21, 2021 · 3 comments

Comments

@hug-dev
Copy link
Member

hug-dev commented Sep 21, 2021

This was discussed on the community meeting, see the related minutes here.

This is about two things:

  1. Activating Squash and Merge as an option on all repos, but not setting it as default.
  2. Adding the merge procedure that the maintainer merging the PR should follow in the book.
@MattDavis00
Copy link
Member

MattDavis00 commented Sep 22, 2021

Might be worth mentioning that pull-request templates may help.

For example:

Markdown:


<!---
Please use the PR template to explain why your pull request is needed,
what changes it makes, and how you would like it to be merged.
-->

### What changes does this PR make?:
Details...

### Issue:
Fixes #ISSUE_NUMBER

### Merge Method:
- [X] Merge
- [ ] Squash & Merge
- [ ] Rebase & Merge

Preview:


What changes does this PR make?:

Details...

Issue:

Fixes #ISSUE_NUMBER

Merge Method:

  • Merge
  • Squash & Merge
  • Rebase & Merge

@hug-dev
Copy link
Member Author

hug-dev commented Sep 22, 2021

This is my personnal opinion but in general I am not a fan of templates 😬.
Even though they are definitely better for maintainers to understand easily what the PR is about, they add more administrative work on the contributor side to fill the required sections.
I think that the general philosophy, at least for now as contributions are low, should be to put all (or most) work on the maintainers' side and as less as possible for the contributor in order to make it as easy as possible for people to contribute. Even people less experienced with Open-source, GitHub workflows, git, etc...
This kind of template might frighten new contributors who already made a long way to publish their PR and who now have to fill the template.
I would say even if the PR description is not great, the commit messages not perfect, let's accept it how it is and work with the contributor to make is better if needed.

About the Merge Method (which was the original goal for the template, sorry to disgress 🙄) I have the same thinking than above: some new contributors might not even know about those merge method and what to do. I would say that for maintainers, in most cases there are two choices what to do:

  1. The contributor knows about git, cares about its commits and always rebase them and force-push after changes. Example: Add functions and types needed for ECDH-based decryption rust-cryptoki#24. In that case, the maintainer should either merge or rebase and merge (better in my opinion if that does not create a merge commit).
  2. The contributor is either new to git, does not care about its commits (which is totally fine in my opinion) or sends a new fix commit for every review step (which is actually quite nice for reviewing). Example: Implementation of PsaGenerateKey and PsaDestroyKey operations parsec#354. In that case the maintainer should ask the contributor if they are ok to squash and merge and then do it.

This is all my personal opinion though!

@MattDavis00
Copy link
Member

MattDavis00 commented Sep 22, 2021

Yeah I agree. I don't really mind either way; I kind of have a love hate relationship with templates. On the one hand it makes sure you get all of the information you need in a structured official format. On the other hand I don't like them because it seems verbose and takes extra time. It also disables stuff like auto filling the commit messages in the PR description which is annoying 😢 was just aiming to voice the suggestion mainly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants