Pull request process

[hokomoko]

In order to make contribution easier both for the contributor and the engine developers that accept it, I thought about formalising the process a bit.
The current description in the wiki is partial at best:
https://springrts.com/wiki/Development: ... e_included

I suggest that any PR will be required to:
1) Be successfully tested locally.
2) Conform with the coding standards
3) Update the changelog if it's a major change - this is not to save work for the devs, but mostly to provide a concise explanation of what the PR changes.

On my part, I plan on responding to PRs faster, and making sure every PR gets attention, so if a ping-pong is necessary it will be as short and efficient as possible.


I mostly wish to discuss "process" requirements and not code requirements (such as having different features split to different commits, not having bugs etc.) which are less of a problem IMO.

I'd love to hear suggestions for the text of the wiki section or anything you think I've forgotten.

Try to keep this discussion practical.

[gajop]

In order to make contribution easier both for the contributor and the engine developers that accept it, I thought about formalising the process a bit.
The current description in the wiki is partial at best:
https://springrts.com/wiki/Development: ... e_included

Also update the wiki when you're done!

I suggest that any PR will be required to:
1) Be successfully tested locally.
2) Conform with the coding standards
3) Update the changelog if it's a major change - this is not to save work for the devs, but mostly to provide a concise explanation of what the PR changes.

All true. Also, changelog should be updated in *most* cases imo, and especially when a new feature was added, important bug fixed or existing functionality changed.

I mostly wish to discuss "process" requirements and not code requirements (such as having different features split to different commits, not having bugs etc.) which are less of a problem IMO.

I'd love to hear suggestions for the text of the wiki section or anything you think I've forgotten.

Keep it polite (remember, these people are contributing to your project!), and make sure you answer each PR fast even with a negative review.
Try to keep PR reviews constructive so people know how to fix their changes if needed.

So basically, just common sense

[jamerlan]

you should probably add that PR:
1) should not have merge commits
2) should have small amount of commits (1 commit - the best choice usually)

[FLOZi]

Agree with all of this except I am useless when it comes to avoiding merge commits.

[jamerlan]

Agree with all of this except I am useless when it comes to avoiding merge commits. :oops:

1) don't do "git merge upstream/develop" but do "git rebase upstream/develop" instead
2) if you have merge commits you can create a new branch (from develop) and cherry-pick your commits (should be 1 usual)

if you have many commits - then squash them using "git rebase -i"

[enetheru]

Subscribing to this thread because I anticipate it being super useful..

[hokomoko]

you should probably add that PR:
1) should not have merge commits
2) should have small amount of commits (1 commit - the best choice usually)

Good points.
What do you think about:
4) PRs should have a small amount of commits, each of them well defined. Please refrain from including merge commits.
?

[jamerlan]

What do you think about:
4) PRs should have a small amount of commits, each of them well defined. Please refrain from including merge commits.
?

Sounds good

[raaar]

Agree with all of this except I am useless when it comes to avoiding merge commits.

1) don't do "git merge upstream/develop" but do "git rebase upstream/develop" instead
2) if you have merge commits you can create a new branch (from develop) and cherry-pick your commits (should be 1 usual)

if you have many commits - then squash them using "git rebase -i"

I have a pull request with several commits and some merges. I had a previous one with one big commit and was told to split into multiple.

It makes sense to split commits about unrelated aspects of the engine into multiple pull requests, but what if they are related?

[gajop]

Normally if you were planning to make all these changes again, I'd definitely split things into smaller commits and get them merged in fast. That said, seeing as how you already have the PR open, I'd just merge it in the next version to simplify the fuss (these are still separate commits which can be tested independently).

A slight digression, adding more ridiculous unitdef tags such as "airManeuverabilitySpread" and "attackOverflyDistance" in your example makes creating new games an unnecessarily laborious task (how to figure out what values make sense for these variables, and when would one change it?). Already it's often advised to make units by copying unitdefs from ZK, BA and other games, and this doesn't help.
If at all possible, the aim should be to make simple atomic concepts that can be used to create complex behavior, rather than a hugely complex system that attempts to cover all use cases.

[raaar]

Normally if you were planning to make all these changes again, I'd definitely split things into smaller commits and get them merged in fast. That said, seeing as how you already have the PR open, I'd just merge it in the next version to simplify the fuss (these are still separate commits which can be tested independently).

A slight digression, adding more ridiculous unitdef tags such as "airManeuverabilitySpread" and "attackOverflyDistance" in your example makes creating new games an unnecessarily laborious task (how to figure out what values make sense for these variables, and when would one change it?). Already it's often advised to make units by copying unitdefs from ZK, BA and other games, and this doesn't help.
If at all possible, the aim should be to make simple atomic concepts that can be used to create complex behavior, rather than a hugely complex system that attempts to cover all use cases.

The point wasn't to discuss the changes, just as an example. I tested my build on XTA and ZK, and a bit on S44 and BA as well. I added the new unitdefs but made it so the default behavior works similar to what we currently have. It enables game devs to control some aspects of aircraft behavior, but doesn't force them to.

EDIT: to discuss this, go here

[Silentwings]

what if they are related?

I'd say split them up anyway, as far possible, since it makes it easier to figure out whats going on when (as in your own example) there are unintended logic changes and bugs.

[abma]

It makes sense to split commits about unrelated aspects of the engine into multiple pull requests, but what if they are related?

then split them into multiple commits. its difficult to write a rule which appies to everything.

maybe missing, pull requests/patches should
1. compile / pass validation test(s)
2. if possible: be a single commit / or few commits
3. when changes are large: split into multiple commits so a review / revert / bisect is easier / possible

[abma]

also hints like this could be added for pull requests imo:

do not add a new def tag for every little highly specific behavior you want to control

use the luasynced(move)ctrl interface instead and make such unit parameters dynamically configurable

tags are only meant for rigid static type properties, they limit the engine's flexibility and extensibility and their number should be decreased with new releases, not increased

viewtopic.php?p=570407#p570407

[Silentwings]

I think advice on making PRs could be more prominent than just a link to a wiki page from https://springrts.com/wiki/Development: ... e_included, imo it almost deserves a link on https://springrts.com/wiki/Main_Page