I'm a big fan of continuous integration. And therefor I quite dislike a Merge/Pull Request process that basically puts people's work in a jail.
MR/PRs (or pre-integration reviews) are often used as a faux gatekeeper to quality, but I think their net effect is zero. Many bugs slip past those reviews, and quickly fixing issues which slipped past is not possible as you have to wait again on those reviews.
Further more, I have yet to see a MR/PR UI which isn't terrible. They focus on changed lines and actively hide context. The presentation gives a really confusing look at what the effective code will be.
I'm trying to optimize our pre-integration review process as much as possible, having tooling there to review the code first.
I hope to eventually come to a place where we can adopt a rule that allows people to self-merge changes if tooling approves.
The problem with diffs is that they focus on changes lines, not changed behavior. As far as I know there are no diff tools around which understand the language and can therefor provide better contextual information. So, it's up to the "viewer" of the diff. The viewer is in most cases a website that just pretty prints the diff. Highlights changed lines and line segments, and adds syntax coloring. Changes can either be presented side by side or unified. Both have major issues:
Side-by-side/split:
serious line wrapping
Unified:
block/control-flow changes are obscured
Both have the problem that they do not understand the language so changed cross method borders. They do not indicate that a method was removed and a new method was introduced. Instead it makes it look like a method was changed/replaced. Especially when the line(s) before the method have identical content.
So structural representation of the diff is line if it was a flat text file, rather than structured source code.
Then there is the issue that you can spelunk the code. You can not open the implementation of a method that is now being called, or is no longer being called.
For example:
function fooBar() {
- initQuux(true);
+ initQuux(false);
calculateQuux();
initQuux() was change... but what is the new behavior?
Obviously you can check out the branch. But then you loose the MR/PR context. Sure various IDEs can highlight code based on commits. But a MR/PR can be a collection of commits.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
I'm a big fan of continuous integration. And therefor I quite dislike a Merge/Pull Request process that basically puts people's work in a jail.
MR/PRs (or pre-integration reviews) are often used as a faux gatekeeper to quality, but I think their net effect is zero. Many bugs slip past those reviews, and quickly fixing issues which slipped past is not possible as you have to wait again on those reviews.
Further more, I have yet to see a MR/PR UI which isn't terrible. They focus on changed lines and actively hide context. The presentation gives a really confusing look at what the effective code will be.
I'm trying to optimize our pre-integration review process as much as possible, having tooling there to review the code first.
I hope to eventually come to a place where we can adopt a rule that allows people to self-merge changes if tooling approves.
Michiel --
You make a good point -- We at LinearB can definitely give this a look and see if we can do something about a better UI.
What suggestions do you have? What context is being hidden -- the code around the changes? How would you improve things?
Nick
The problem with diffs is that they focus on changes lines, not changed behavior. As far as I know there are no diff tools around which understand the language and can therefor provide better contextual information. So, it's up to the "viewer" of the diff. The viewer is in most cases a website that just pretty prints the diff. Highlights changed lines and line segments, and adds syntax coloring. Changes can either be presented side by side or unified. Both have major issues:
Side-by-side/split:
Unified:
Both have the problem that they do not understand the language so changed cross method borders. They do not indicate that a method was removed and a new method was introduced. Instead it makes it look like a method was changed/replaced. Especially when the line(s) before the method have identical content.
So structural representation of the diff is line if it was a flat text file, rather than structured source code.
Then there is the issue that you can spelunk the code. You can not open the implementation of a method that is now being called, or is no longer being called.
For example:
initQuux()was change... but what is the new behavior?Obviously you can check out the branch. But then you loose the MR/PR context. Sure various IDEs can highlight code based on commits. But a MR/PR can be a collection of commits.