Based on recent comments in this Forum and my own experiences, I believe there is frustration around the PR process. Specifically, pull requests to our main Engine repository (https://github.com/jMonkeyEngine/jmonkeyengine).
I suspect people are coming to the PR process with conflicting expectations. Perhaps the PR process would go more smoothly if we established some common expectations—or if we were more aware of where our expectations diverge.
Questions to ponder:
Who may give feedback and perform code review on pull requests?
Who may approve pull requests?
Who may integrate pull requests?
Under what circumstances may changes be committed directly to the “master” branch without a pull request?
Under what circumstances may PRs be integrated without explicit approval?
Under what circumstances may PRs be self-integrated (integrated by the submitter)?
May a submitter request feedback or review from specific persons, or request that specific people not review their PR?
What should happen if the submitter is slow to respond to feedback or code review?
What should happen if the submitter is slow to make requested changes?
May a submitter ask reviewers to make their requested changes?
Naturally, I have my own opinions on these questions. However, I wish to hear from other interested parties before sharing my views.
Who may give feedback and perform code review on pull requests?
May a submitter request feedback or review from specific persons, or request that specific people not review their PR?
One thing I’ve found useful at my workplace is a codeowners file. Because often the problem with JMonkey is to figure out who has the knowledge to review a change (lots of people know about parts of the code, but very few people know about all of the code). The way we work it is that the codeowners file defines paths that if they are changed suggest a set of default reviewers. It doesn’t mean those people have to review it (or that an approval from someone else isn’t good enough) just that those are the people worth pinging
May a submitter ask reviewers to make their requested changes?
That feels like a big ask. If the reviewer really wants to then fine, but it feels like it shouldn’t be the expectation
Anyone within the community who possesses knowledge of the code changes is encouraged to review the Pull Request
If a reviewer is confident that the PR requires no further alterations, they are encouraged to provide their approval from the github interface
Bug fixes can be self-integrated if the person doing it agrees to fix any unexpected issues that might arise and that could have been prevented with more discussion
For feature PRs, they need approval from at least one core developer (two if it’s from a core developer). Everyone is encouraged to review, and if someone lacks the expertise (or time), they can still express approval or disapproval by reacting with or to indicate their stance on the feature’s general concept.
If the submitter is slow to respond, we just wait
When you submit a pull request, you can choose to let repository members with push rights to make commits to it. If a submitter allows this, we should consider it as an implicit permission. Reviewers with push rights can decide to make changes to the submitter’s branch at their discretion , but they should do so only at the end of the process for minor tweaks (like forms or adding tests) to avoid disrupting the submitter’s work if they are still working on the PR or making requested changes.
One thing you didn’t address in your list is that one striking comment I recently saw here was about the importance of formatting (I read this comment as formatting is more important than substance). This I think includes the up-to-date license headers and all that. It would be absolutely superb if that was done by some automation. That is also easier for people to agree on, instant feedback from a robot (static code analysis etc). Also the auto-formatter thing was ok, but it should be then perhaps applied to the whole code base once the rules are set, then it wouldn’t mess the PRs as there would not be any unnecessary changes that interfere with the readability of the PR.
In all, I think uniform code / formatting is fairly important thing and can be enforced in the PRs. Should also be a small thing for an author. Of course, bedside manners and all that. It is very easy to offend people, giving and receiving feedback is really a skill. Hard to master.
Most important thing is to react to PRs, any reaction. So that we tap into the fresh energy of the author. PRs go stale and motivation is easily lost. Good job @RiccardoBlb and @sgold for being fast on commenting the PRs.
Yes, auto-formatting is the only answer to where we find ourselves. That means that we’ve officially decided that draconian formatting rules are more important than readability (“a foolish consistency, etc.”)… for a dozen really good reasons individually. Though those reasons, taken as a whole, eroded the original point of “code is more readable when it’s not messy”. (The biggest problem being the propensity of IDEs to auto-format a file just because you touch it leading to format wars in the commit history.)
The only unfortunate thing about auto-formatting the whole repository is the “git blame” choke point it causes. (Bad enough sometimes that the web-based github UI cuts history off when we transferred from google code…even though the history is still there.)
But it is necessary. It’s the only way to avoid throwing the person who reformats the whole file to change one “if” statement into the same pot with the person who made 100 lines of superb changes but forgot to put a space before one curly brace.
The use-case is when you know there is an expert on the team and you’d feel better if they approve your changes. This happens all the time even at my day job. It doesn’t mean that others can’t also approve… but it’s a good practice to let an expert review your changes, especially if you are new to a section.
For those using Visual Studio code , there is a new feature that can format changed lines on save, i’ve enabled it in the jme repo. I think there might be similar settings for other environments.
I agree that having an automated workflow for PRs would be great, but I couldn’t find an easy way to target only changed lines. I tried with a workflow that targeted changed files but we have seen that this was creating too much clutter.
After reconsidering the idea of reformatting the entire repository, I came across this: GitHub Docs - Ignore Commits in the Blame View. It seems this can solve the git blame issues, are there other points against reformatting the entire repo?
Well, this rule would be very hard to respect.
It means if a line is poorly formatted i should pay attention to keep it this way and disable every formatting tool of my editor.
I would make an argument for finding the bare minimum set of rules that really need to be enforced (e.g. java style braces, spaces not tabs, 4 space indent) and only having those rules.
Arguing over if
if ( a < b ) {
or
if(a<b){
is better doesnt benefit anyone (and wildly alternating between the two doesnt hamper readability one bit)
And some other rules (like if an else with a return is acceptable as the last branch in a method) actively hurt readability because sometimes one better expesses the intent and sometimes the other does (even though mechanically theyre the same)
Ok, but you assume that everyone who wants 2 is writing code following the rules or remembers to trigger the auto format.
The workflow would make impossible to commit code that doesn’t respect the rules, resolving all formatting disputes once for all, assuming we find a way to implement it without destroying the repo history.
I understand that you might not see the utility in this, especially since I’ve seen your code (as well as @pspeed 's), and you both consistently produce well formatted code. But many of us are affected by formatting blindness (not an actual thing, i made that up).
I’d wish I could easily assist you with formatting.
One technique I use is to auto-format an entire file, then go back and undo all changes that aren’t relevant to the PR. However, I always do this before committing any changes. I find it much harder to do this after changes are committed, so it’s not something I’m willing to do for many PRs. Perhaps I just haven’t found the right tool.
The workflow would make impossible to commit code that doesn’t respect the rules, resolving all formatting disputes once for all, assuming we find a way to implement it without destroying the repo history.
I’m puzzled by this part. The rules you refer to, are those merely the formatting parts of the style guide?
Our preferred style isn’t designed to enforced mechanically. Most of the formatting aspects can be, sure. But I wouldn’t trust software decide which members are so simple and self-explanatory that they don’t need javadoc. Or apply camel case correctly to identifiers containing abbreviations. Or reliably convert sentences to equivalent fragments. Not unless it was a very carefully trained AI.
Even with basic formatting like whitespace, it’s easy to ruin readability while conforming the style guide. For instance, putting every method argument, regardless of length, on a separate line. That’s legal, but seldom good for readability.
On the other hand, I don’t recall seeing formatting disputes in JME pull requests. If PR authors resent being asked to add whitespace or braces or javadoc, they usually don’t say so. Mostly they just comply. In most cases, it’s easier to make the requested edits than to argue.
The risk is that unexpressed resentment might discourage people from contributing in the future.
Yea, also in my work place we use codeowners file and the good thing about it is that sometimes it enforces more than 1 owner to approve the code (imagine a big PR affecting many zones in the project).
Also:
We never ever allow merging to master without code review.
We work with NodeJS & Typescript so JSLint is used to enforce formatting etc. and it’s very strict.
We have zero warnings, zero errors policy for commting a code.
Merge to master involves auto triggering unit + integration + static code analysis + security tests (all running via Jenkins jobs) which must pass or it wont merge.
We consider it an honor to be reviewed by a senior member and improve ourselves.
IMO It’s important for JME leaders to response fast to important PRs and help the author to merge his features.
It’s very exciting to see all those new features developed for the engine by great developers - thank you!
I would say that javadoc, camel case errors and string splitting is not the problem. (at least for me). I would say this is a user problem and has to be fixes by the user.
For me it is the 3 spaces instead of a tab, here a white space, there a new line before curly. and so on. License header could also be auto included i think.
This are the things the ide autoformatting messes up, and takes lot of time to fix.
I have been there, and it just kills the mood and the motivation to make changes. I know it is a problem at my side, but it would be nice to have a solution there.
Even i don’t recall what the key combo for ‘auto format’ is in idea, but i am pressing it all the time
We could try this. It would be more thing to maintain, of course. Would you want the job of maintaining the file? Without complete buy-in, it would be like herding cats…
In my experience, GitHub automatically suggests a reviewer for each PR. However, its suggestion is seldom the person I would choose.
My preferred reviewer has little to do with familiarity with the affected code. It’s someone skilled at code review and likely to act in a timely fashion. The latter factor varies from month to month, depending on who’s active in the project.
Personally, I don’t enjoy reviewing code. I’m not good at it yet. Most of the time, I do it only because nobody else has stepped up. Asking someone to do it feels like a big ask.
I agree with standardizing code formatting. In projects I have participated in, each project would have its own unified conventions, this is basic. However, writing comments can be an extremely time-consuming process, it may take more time than writing the code itself. There could be debates on whether every function needs detailed comments (for example a public String getName(){return name;} kind of getter/setter functions may not even need comments). Anyway, if mandatory requirements on submitted code are specified (formatting, style, comments, etc.), then modifications should be made following the specifications until requirements are met.
I noticed that in my latest PR (also my first submitted PR), I did not write extensive comments for every function, this is something I need to improve on (well, I took a shortcut , I thought writing detailed comments is much more tedious than writing the code itself, it may even take more time than developing the functionality…). If my PR has other issues not meeting submission requirements, please let me know, thank you.
or 
to indicate their stance on the feature’s general concept.
(not an actual thing, i made that up).
, I thought writing detailed comments is much more tedious than writing the code itself, it may even take more time than developing the functionality…). If my PR has other issues not meeting submission requirements, please let me know, thank you.