FreeCAD Contribution Process

[GeneFC]

Yes, C4 states that if you dislike the PR about this you should make a PR of your own rather than advocate for not merging that one

Battle of the PRs is exactly what should be avoided by applying "judgement." It is hard to believe that any successful FOSS project could work under that model.

Gene

[sliptonic]

I want to respond to a couple of things that I'm hearing:

First, the C4 process is our inspiration and our starting point but we are not implementing C4. We're taking our first steps to define our own process and live by it. Our process is documented in CONTRIBUTING.md. That document and the process are subject to change through the very process it defines.

We will not be able to automatically apply this process retroactively to existing PRs and issues. Toponaming is in-process and we're just going to have to muddle through. This is going to take time. We're going to make mistakes.

We can (and SHOULD) start improving the existing issues to more cleanly and narrowly define problems.

The challenge from all of this recent discussion is "who defines what constitutes a problem?".

We do. Collectively. That's the entire point of an issue/problem. To define and establish consensus on the problem before implementing a solution.

The process tries to avoid the value judgments in the PR but it does NOT avoid those judgments in the Issue. We should be opinionated on the issue. We should debate vigorously about what the problem is and what a solution should look like. We should push that definition to be as narrow as possible.

Yes, C4 states that if you dislike the PR about this you should make a PR of your own rather than advocate for not merging that one

Battle of the PRs is exactly what should be avoided by applying "judgement." It is hard to believe that any successful FOSS project could work under that model.

And yet, projects do work under this model. If everyone is acting like adults and seeking the best outcome for the project, PR battles are extremely rare. When it happens, it's because the problem wasn't properly defined.

Werner's comments above are spot-on. For smaller changes, merging is not a problem. For larger PRs with design problems, merging could be a mistake. But that's a hard call to make, even for really experienced developers. So simply punting it to the maintainers and asking them to use 'judgment' isn't good enough. We need some guidelines.

I think the proposed solution that Werner and I discussed above is a good starting place. I've created an issue about that and will make a PR amending the policy.

this is brings us back to the previous point about requiring an issue before making a PR, if one makes a PR directly without creating an issue can the value of the contribution then be discussed in the PR? or do we then ask that an issue be created for such discussion?

This is really tempting. As a developer trying to "eat the dog food" myself for the last few weeks I've seen it first-hand. I stumbled across some small problem. It's a single-line fix. To go and create an issue, then code the solution, then create the PR seems like too much work and artificial. I don't have a good answer. Maybe there's some size below which a PR only is ok. I would suggest that we NOT answer this one just yet. Let's try living with the problem a little while before we propose a solution.

I will say that while doing a PR only is very tempting, it's also dangerous. The entire point of the process is to have two different conversations. One about the problem and one about the solution. By allowing PR-only discussions, you are saying that those are the same discussion. They're not.

[Zolko]

We will not be able to automatically apply this process retroactively to existing PRs and issues. Toponaming is in-process and we're just going to have to muddle through. This is going to take time. We're going to make mistakes.

I think that that's a wasted opportunity: we should use the toponaming merge as a test-case for the future contribution policy. Yes, mistakes will be made, but by documenting and learning from those mistakes there will be a battle-hardend procedure that can last for long time. We should use that task as a pathfinder for good practice.

[adrianinsaval]

I don't have a good answer. Maybe there's some size below which a PR only is ok. I would suggest that we NOT answer this one just yet. Let's try living with the problem a little while before we propose a solution.

I will say that while doing a PR only is very tempting, it's also dangerous. The entire point of the process is to have two different conversations. One about the problem and one about the solution. By allowing PR-only discussions, you are saying that those are the same discussion. They're not.

I say if someone makes a PR only and there's nothing to question it's fine to just merge, but if we are going to have a discussion about the problem it's trying to solve and the value of solving it then we create an issue and discuss there. We can do this as a stop gap measure while we think of better policy after more experience or put it in the policy directly if you want. I expect the policy to be an evolving document anyways so if something there doesn't work we'll change it.

[Roy_043]

"eat the dog food" myself for the last few weeks I've seen it first-hand. I stumbled across some small problem. It's a single-line fix. To go and create an issue, then code the solution, then create the PR seems like too much work

To further my understanding of the Contribution Requirements, can you give an example of such an issue and the subsequent PR?

[sliptonic]

To further my understanding of the Contribution Requirements, can you give an example of such an issue and the subsequent PR?

We're pretty new so I can't say I did a good job on either the issue or the PR. This looks like more than a 1 liner in the code but it's literally just changing the name of the controls in the .UI file

https://github.com/FreeCAD/FreeCAD/issues/7652
https://github.com/FreeCAD/FreeCAD/pull/7653

[Roy_043]

Thanks, but this leaves me somewhat confused. Both the issue and the PR have a single participant. How can the issue then be an agreed-on problem? My current, maybe too strict, interpretation of the Contribution Requirements is that an issue has to be officially approved before a PR can be submitted.

[adrianinsaval]

IMO something like that is not worth creating an issue if you already have a PR ready, if you intend to document the issue and work on the PR later or hope that someone else tackles it it makes sense.
About requiring consensus... once again, seems excessive for something so trivial, specially if it is a regression! but I would say there is consensus on the linked forum thread for the issue, at least three people agreed. The PR... this is one of those things that would just give unnecessary delay if it actually requires that someone else merges it. We need to have this kind of exception or we're just increasing work with no benefit. The tricky question is where to draw the line.

[sliptonic]

Thanks, but this leaves me somewhat confused. Both the issue and the PR have a single participant. How can the issue then be an agreed-on problem? My current, maybe too strict, interpretation of the Contribution Requirements is that an issue has to be officially approved before a PR can be submitted.

This is exactly the rub. The C4 process, from which we take our inspiration, tries to remove ambiguity in the process and avoid conflict downstream. The result is a process that feels legalistic and artificial for trivial things.

The alternative is a process that only requires issues for "non-trivial" things. But that requires judgment or definition about what is 'trivial'.

We don't have a good answer. There probably isn't one. We'll probably end up with a set of guidelines and 'best practices to help decide. Some possible examples off the top of my head (only examples):

- A first-time contributor SHOULD NOT send a PR that doesn't have a corresponding issue, regardless of how trivial.
- An experienced contributor MAY send a PR without a corresponding issue if the PR touches only one file and is less than ~50 lines of changed code.
- A maintainer MAY create a PR without an issue for a fix that they deem 'trivial'
- A PR without an issue SHOULD be left for at least 24 hours to give time for review and comment.

[adrianinsaval]

maybe add to that policy:
- Critical or security bug fixes MAY be merged directly by maintainers