[Problem] The contribution policy allows for structurally problematic code to be merged.

[sliptonic]

Is there an existing request for this?

• I have searched the existing requests

Forums discussion

https://forum.freecadweb.org/viewtopic.php?f=8&t=73127&start=30#p639075

Subproject(s) affected?

No response

Idea description

Werner raised the concern that the defined process places too much bias on merging a PR. If the PR passes tests, solves a problem but includes dangerous code-smell or design issues, there are times when it should rightfully be held.

We propose to modify the policy to allow a PR to be held in this case if at least two maintainers agree that it should not be merged. This avoids a single maintainer becoming a gate-keeper to code acceptance.

Anything else?

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[chennes]

If one maintainer wants to merge, and a second does not, how long do we wait to see if a third maintainer weighs in?

[sliptonic]

If one maintainer wants to merge, and a second does not, how long do we wait to see if a third maintainer weighs in?

Do you think that needs to be spelled out? I expected this would play out that a Maintainer, looking at the code, has misgivings and asks other maintainers for opinions. They (we?) would be seeking a consensus opinion on moving forward.

[chennes]

You're probably right, I'm just trying to think through the implementation challenges. If the system is working, then yes, a maintainer with misgivings asks for a second opinion, gets one, and we move on. The failure mode I'm worried about is a single maintainer with misgivings says they have misgivings, opens it for commentary, and gets no further responses. Is the maintainer with misgivings then supposed to merge anyway?

[WandererFan]

One Maintainer being uncomfortable should be enough to challenge a PR. A "challenged" PR should then need the agreement of two Maintainers to proceed.

[sliptonic]

The overall bias of the policy should be toward merging. In other words, unless there's a good reason to not merge, you merge. Holding back a PR should be the exception and the burden of proof should be on the maintainers.

IMHO, a single developer's misgivings about code quality (given that the PR already passes tests and solves a previously identified problem) isn't a high enough bar.

If two maintainers can override the first one, It sets the system up for political lobbying and taking sides and other kinds of mischief.

[WandererFan]

You'll get conflict any time a merge is disputed under any system. Let's try it as written and see how it works. We can always change the rules if they don't work out.

[Zolko-123]

unless there's a good reason to not merge, you merge. Holding back a PR should be the exception and the burden of proof should be on the maintainers.

I don't agree. There is a universal good reason not to merge: maintenance. Take the workbench selector that some people wanted to put in the menubar: since it was optional, it could be assumed that merging it wouldn't cause any problems. But then, in a few years time, when Apple or Microsoft change their API, suddenly this will stop working, while the Qt part will work. Then, somebody else must clean up the mess that the initial contributor created. This will very fast lead to bloatware.

I will actually argue the opposite: if it ain't broke don't fix-it. The burden of proof is on the contributor to show that 1) his contribution does indeed solve a real problem and 2) that his contribution will easily stand the test of time.

[sliptonic]

I don't agree. There is a universal good reason not to merge: maintenance. Take the workbench selector that some people wanted to put in the menubar: since it was optional, it could be assumed that merging it wouldn't cause any problems. But then, in a few years time, when Apple or Microsoft change their API, suddenly this will stop working, while the Qt part will work. Then, somebody else must clean up the mess that the initial contributor created. This will very fast lead to bloatware.

I will actually argue the opposite: if it ain't broke don't fix-it. The burden of proof is on the contributor to show that 1) his contribution does indeed solve a real problem and 2) that his contribution will easily stand the test of time.

For item #1, You're not wrong but this is already covered by the policy. We can reject a PR that doesn't have an identified / agreed problem.

For item #2, The policy says nothing about that. How would you judge whether a contribution will stand the test of time? Discussion of implementation can be had in the PR prior to merge and as long as the contributor is willing to improve it, there's no problem.
If the contribution solves a real problem, passes muster with Contributors and passes tests, it should merge. Beyond that, it would fall into the category covered by the policy in 6.15 "Any Contributor who has value judgments on a PR SHOULD express these via their own PR."

That said, The issue that Werner has raised still stands. If a contribution has enough deficient code to be concerning to multiple maintainers, the policy should allow it to be held.

[Zolko-123]

How would you judge whether a contribution will stand the test of time ?

it can't be judged in a precise way of course, but there can be indications like required dependencies: the more a code depends on something else, and even more if it depends on external code, the likelier it will be that it breaks with time. Or the opposite: the more self-contained a code is, the more robust it will be.

Quantitatively, one can look at the amount of #includes it contains, and where they come from : from the same package (directory), from some other FreeCAD code (Base, App...), from some external code shipped with FreeCAD (3rdParty...), from some external library supplied by the OS (Qt, Python, Coin3D ...). Anything adding an #include and/or #DEFINE should receive additional scrutiny and should conform to higher standards because it raises the overall complexity and the dependency graph.

This could even be written down: any code adding a new #include would need the approval of 2 maintainers at least.