Pull request merging policy

minetest / minetest_docs

Minetest Engine documentation (Work in progress. Meanwhile, please refer to lua_api.txt and dev.minetest.net)

Other

22 stars 13 forks source link

Pull request merging policy #19

Closed appgurueu closed 2 years ago

wsor4035 commented 2 years ago

a empty policy? hardly seems useful

benrob0329 commented 2 years ago

"Our one policy is not having a policy"

appgurueu commented 2 years ago

a empty policy? hardly seems useful

I was just trying to get the discussion started. But if you want my proposal:

Squash if the history is messy;
Create a merge commit if it's not

JosiahWI commented 2 years ago

To add to that, it would be beneficial to work on making good commits that don't need to be squashed, since this is a learning experience for me and maybe for you too. Forming good habits helps us develop efficiently, and shows that we care about the project.

My suggestions are:

keep the changes in a commit directly related to a single goal
review your changes before you commit them; git diff is your friend
keep commit messages brief, and do not capitalize the first letter
if you need to explain more, put it in the body of the commit

While writing this document, I followed the plain English practices Benrob shared with us in the IRC channel. You can read them here: https://www.plainenglish.co.uk/how-to-write-in-plain-english.html

benrob0329 commented 2 years ago

To put my 2 bucks in the jar:

Every PR must have at least 1 non-contested approval from another Minetest-Docs team member (other than the PR author) to be merged
Have useful, scoped commit messages. Such as: vector - clarified metatable function usage
If a PR branch falls behind master prefer rebasing PR branches if it can be done cleanly, use a merge commit otherwise
Commits can be renamed before merge for clarity while the context is fresh
Prefer not squashing messy PRs into one monolithic chunk. Rather, squash specific commits together if applicable

benrob0329 commented 2 years ago

We probably need a policy for when to close PRs as abandoned/stagnant. I'd argue that if a PR sits around with no activity from the author(s) for 2 weeks, it aught to be closed until the author(s) request a re-opening.

erlehmann commented 2 years ago

@benrob0329 I think that is an extremely counterproductive idea – in fact, I think it is the worst proposal regarding Minetest documentation I have read so far. I have seen a lot of PRs in a variety of projects (first of all, Minetest) that have been neglected for weeks or even months only to be accepted. IMO automated closing of tickets or PRs based on how long they have been open is one of the most toxic things that a project can do.

I suggest to only close abandoned PRs once a replacement PR is created or they are no longer deemed necessary.

benrob0329 commented 2 years ago

Our goal is to try to get PRs merged if beneficial, but if they die unfinished and no one has time to then they shouldn't sit around hopelessly. It's also not a matter of how long it's been open, not at all. It's how long they've been neglected by the author(s). Being closed is also not a rejection, it's saying "this isn't open for merging at this time". If a PR is rejected then that's a whole 'nother thing entirely. I think that we should be very willing to re-open PRs if they become active again and see progress, and that contributors should feel open to request a re-opening at any time should they return to it.

wsor4035 commented 2 years ago

I agree with the policy presented by benrob. (aside from a delay till the formatting pr is merged). [suggestion] additionally all closed prs should be fine to reopen unless labeled with something like rejected/wont fix/etc label

appgurueu commented 2 years ago

I guess we have informally agreed on how to merge PRs anyway. @GreenXenith can you disable merge commits?