Contributing/Review
Proposal
This is a proposal, things may be discussed and changed.
See the forum thread: https://forums.unvanquished.net/viewtopic.php?t=2426
Contributing, Reviewing, and Merging
Here are some recommendations to better deal with contributions.
A contribution is done in three steps:
- Producing a change, see Contributing/Code and Contributing/Media;
- Submitting the change to review and reviewing it, see Contributing/Review;
- Merging the code, see Contributing/Merge.
The purpose of those proposed recommendations is to:
- Reduce the risk of pull requests being stuck in discussions or waiting for approvals to be merged.
- Reduce the risk of people being afraid of merging or approving or submitting a pull-request.
Contents
Reviewing artistic data
Reviewing images, materials, sound effects or map changes from UnvanquishedAssets is a bit different than code, as there are some art direction involved.
A data PR modifying how a the game world is experienced, for example how a map looks, is expected to get approved by an artistic advisor before merging. Common artistic reviewers are currently: Viech, illwieckz, Sweet.
An artistic data author is also a reviewer for his own map / texture / other artistic data package, for example the author of a map is considered an artistic reviewer for the map, same for texture packages if the author is part of the Unvanquished team (a texture package specially created for Unvanquished) and same for sound. For model repositories like weapons, buildables and players, the author of a model is considered an artistic reviewer for the things related to the model.
Artistic reviewers can say a change literally looks good to them and is OK for merging for the matter of the way it looks.
Artistic reviewers can auto-approve their change for the matter of the way it looks.
Then comes some generic implementation review, basically things like if the syntax of files is correct, or used file format is good or not.
Let's give some use case as an example:
- Someone contributes a force field effect modification by adding a heat haze map.
- It is required to get an artistic review. Artistic reviewers can auto-approve themselves.
- Usual reviewers of the team (same as code) can review the implementation, for example suggesting to compress the image better or writing the
.shaderfile in a better way.
- Someone edits some brushwork in a map without any impact in the perceived geometry.
For example to caulk some surface to optimise the rendering, or to reduce the light map size.- This doesn't require an artistic review, so any usual reviewers of the team can review that.
- The PR submitter should say it doesn't require artistic review and why.
Like code, we better wait for at least one complete day before merging a PR to make possible to gather some comments.
Something specific to data contribution is the file size, because most data formats (image, sound files) are not diffable, we better make sure a file is in its optimal compressed format prior to pushing it into a repository, this is a technical review, not an artistic review.
Any pull request should be reviewed, even if a formality, just to make sure at least another eye seen it before merging. The idea about authors auto-approving themselves only counts for the artistic side of things. The idea is that if the change got reviewed, but no one said anything about the artistic side of things, a map author can consider it's fine to merge without artistic comment. This is to solve the problem of no one having any opinion on the artistic side of the given change, but everyone being able to say the change is technically ready to merge.
Reviewing code
Reviewing code doesn't require an artistic review. All members of the team are reviewers. The review focus on the contribution being bug-free, maintainable, things like that.
Requesting a review
When submitting a pull-request, it is useful to explain not only what the code is meant to do and why, but also express the state of the work you submit and the kind of review you are looking for, in hope this helps to reduce the pull request being trapped in situations that were not intended that would slow down the review process.
Here are some questions you are invited to answer when submitting your work:
Those changes are implementing:
- ☐ A fix.
- ☐ A new feature.
- ☐ A rewrite of something that already exists.
- ☐ A clean-up to drop legacy or unused stuff.
Those changes are:
- ☐ Made by me.
- ☐ Made by someon else:
What the changes are for and what are they trying to accomplish:
The existing issues related to those changes:
My own personnal perceived status of the changes being submitted:
- ☐ I believe it is ready to merge as is unless proven wrong.
- ☐ I believe it is ready to use but I'm requesting for comments so it can be refined.
- ☐ That's a work-in-progress, being made public to get early comments and make sure other peoples working on other branches know this is being worked on.
I'm looking for this kind of review:
- ☐ Generic proof-reading for mistakes.
- ☐ Comments about the the design itself, looking for better ways to do things.
- ☐ Opinion on what is expected to be brought or removed, is this wanted to begin with? if yes are there better ideas?
- ☐ Opinion on the way it's done or what it plans to achieves.
- ☐ Testing to be sure it works as expected or not regress.
What I already tested and assume is already verified:
I request testing:
- ☐ On a specific operating system, hardware brand or model, driver version:
- ☐ Of specific game options, configurations, in-game actions:
My changes are expected to integrate this way in the target branch:
- ☐ As a preparation for future changes that would be submitted in another pull request.
- ☐ As a single commit despite this being submitted as separate commits.
- ☐ As separate commits because I believe it's better for this change to do things step by step and I don't want to lose the ability to bisect those steps.
Reviewing a contribution
When clicking the “Approve” button, please add a comment, a LGTM (Looks Good To Me) mention is enough.
A LGTM message without any other comment is considered as an approval, better click Approve anyway.
A LGTM message with a comment for a better implementation (not a bug fix) is considered as an approval, better click Approve anyway.
If the code looks to have no bug from you point of view
please write LGTM in all cases.
If you agree with the purpose of the change
Please write LGTM and Approve.
If you disagree with the purpose of the change
Please write LGTM but add a comment explaining why you don't approve.
This way, if the code has no bug, we can solve the politics separately and if we decide to agree with the purpose of the change, we can merge it without having to look back at the code, and to not block the merge because we don't know if code is reviewed yet.
If you have a suggestion for a better implementation
Please write LGTM and approve and add a comment with your suggestion, saying it is a suggestion.
This way, the submitter can decide to follow you or not.
If the submitter merged without following your recommendation, you are free to implement your recommendation.
If the code has a bug from your point of view
If you know the fix and this is obvious to do
Please state what you believe is a bug and suggest what you believe would fix the bug.
You can write “LGTM if you do <suggestion>” and then write your suggestion.
You can consider obvious a fix you believe there is no need to verify the implementation of your recommendation.
Once the submitter implements your suggested fix the change will be considered fully approved without needing any new comment.
If you don't know the fix or this is not obvious to do
Please explain why you believe is a bug, and if you an idea to fix it, please share it.
If you have any remark but it is not blocking
Please write LGTM, approve, and write your remark.
Suggesting a change
Prefer to point out something that could be better, or suggest a change, than give a direct order, because nobody is paid for that work and then everything already done is already a gift that we not deserve.
For example instead of something like:
Remove leading spaces.
Prefer something like:
That leading space will be included in the translation string.
Basically, instead of saying what should be done, say why something can be done.
Approving the change
When you believe the submitted contribution of someone else is ready to merge, you can approve it.
By approving someone else contribution, you help a lot this project! Every merger is an approver.
