FreeCAD Contribution Process

[sliptonic]

Ideally, the maintainers are not judging ... the quality of the code.

Why not?

I should have been clearer. The maintainers shouldn't judge the PR worthiness based on the quality of the code. Of course they should code-review, encourage the contributor to improve it, point out better ways, etc. Most contributors want to learn and improve. The PR discussion can continue as long as both contributors and maintainers feel it's helpful.

The rule is there to answer the question: What do you do when a PR 1) passes tests, 2) solves a problem, and 3) the contributor isn't interested in improving it further?
In that case, it should be merged, even if the maintainer doesn't like the code. If the maintainer feels strongly enough, they can respond with their own PR to improve the code. Maintainers shouldn't become gate-keepers.

Code quality is enforced by unit tests and encouraged/improved through code review.

Unit tests enforce the correctness of the code but not its quality because bad written code may still pass all tests. And who does the code review if not the maintainers?

That's better and more accurate. Anyone can code-review. We should encourage a culture where lots of code review is done. But it should be done with the idea of improving skills and coaching, not pass/fail on the PR.

[wmayer]

Thanks for the clarification!

I should have been clearer. The maintainers shouldn't judge the PR worthiness based on the quality of the code. Of course they should code-review, encourage the contributor to improve it, point out better ways, etc. Most contributors want to learn and improve. The PR discussion can continue as long as both contributors and maintainers feel it's helpful.

I agree on this.

The rule is there to answer the question: What do you do when a PR 1) passes tests, 2) solves a problem, and 3) the contributor isn't interested in improving it further?
In that case, it should be merged, even if the maintainer doesn't like the code. If the maintainer feels strongly enough, they can respond with their own PR to improve the code. Maintainers shouldn't become gate-keepers.

This depends on the quality of the code. Most contributors make PR that aren't too complex and even if the code could be improved it's OK to still merge it. However, if the maintainer sees problems then this should be stated somewhere (e.g. a new ticket or in the forum) so that it can be addressed and improved in the future.

However, sometimes contributors make a complex PR that also solves a certain problem but the code has some serious structural problems (design flaws, code smells of all sorts, ...) then it should be hold back until it has been improved sufficiently as otherwise once merged it will become more difficult to fix it later or even worse it will never be fixed.

That's better and more accurate. Anyone can code-review. We should encourage a culture where lots of code review is done. But it should be done with the idea of improving skills and coaching, not pass/fail on the PR.

Absolutely agree on this.

[sliptonic]

The rule is there to answer the question: What do you do when a PR 1) passes tests, 2) solves a problem, and 3) the contributor isn't interested in improving it further?
In that case, it should be merged, even if the maintainer doesn't like the code. If the maintainer feels strongly enough, they can respond with their own PR to improve the code. Maintainers shouldn't become gate-keepers.

This depends on the quality of the code. Most contributors make PR that aren't too complex and even if the code could be improved it's OK to still merge it. However, if the maintainer sees problems then this should be stated somewhere (e.g. a new ticket or in the forum) so that it can be addressed and improved in the future.

However, sometimes contributors make a complex PR that also solves a certain problem but the code has some serious structural problems (design flaws, code smells of all sorts, ...) then it should be hold back until it has been improved sufficiently as otherwise once merged it will become more difficult to fix it later or even worse it will never be fixed.

This is where it gets tricky as a policy. Who makes the call if the problems are trivial or serious? What criteria should be used? It's almost impossible to define but necessary or PRs will get stuck in limbo.

The original C4 takes a hard line: Merge it.
Our existing process puts all the responsibility on Maintainers: They judge.

Perhaps we should find a middle path: We set our bias towards merging but if two or more Maintainers agree that the problems are significant, it can be held.

[wmayer]

Perhaps we should find a middle path: We set our bias towards merging but if two or more Maintainers agree that the problems are significant, it can be held.

This looks like a good way to go.

[onekk]

Probably the contribution process should be integrated by some "real documentation" about sources, as in the current state is very difficult to guess some basic things when facing the problem of "modifying FreeCAD sources".

It is not impossible to guess some thing and usually when you ask someone will answer, (Thanks @wmayer for you presence in the user forum.) but there is no proper documentation is present as many things are present only in forum messages, some of them very old, and searching through forum messages is not a "cup of tea".

Some sort of tutoring for new developers should be "taken in account" , probably issuing "judgement about the code quality" when a new contributor (or even an old contributor make a PR) has undocumented or poorly documented code should be avoided.

So probably even some "rules about commenting code", maybe using two "standards" one for C++ and one for Python, should be introduced and maybe this will be useful even to create some "automatic documenting" of the code, like in the "help()" method in "Python Console" many functions have no description at all about parameters, or simply list parameters without explaining them.

Regards

Carlo D.

[adrianinsaval]

it's a "solution " to a non-existing problem

the problem is described in the PR and the thread, that you personally did not have this problem is ok, but that does not mean the problem doesn't exist. Personally I did not experience this problem either, I came from CATIA so was familiar with the concept of workbenches, but I have seen many times new users confused about what a workbench is or how to switch from one to the other or how to know which one they where on, the workbench selector clearly did not catch their attention. IMO the problem is real, another real problem is that the toolbar can get overcrowded in some workbenches and saving some space by putting the WB selector somewhere else helps with this. Personally I don't want the WB selector on the menu (or rather I don't want this selector at all as I use selector toolbar addon), but this doesn't invalidate what paddle is trying to achieve.
And the idea that everyone is just too polite to tell paddle when one of his ideas isn't good is ridiculous and utterly false. People have been rude and criticizing of even good ideas and when he presented bad ideas it was quickly pointed out. For example he wanted to remove sketcher from the workbench selector, nobody doubted in expressing this was a bad idea.

[adrianinsaval]

Decisions about the 'value' of the contribution should be pushed upstream to the issue where it is entirely appropriate to discuss whether a problem is worth solving.

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?

[GeneFC]

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

If something is irrelevant to FreeCAD or is simply personal preference it should not be allowed to go forward just because the code is OK. Judgement here is essential to avoid someone deciding to create a function to shop on Amazon, for example.

Personal preferences, such as the recent flap about recoloring the main screen, also fall into the judgement category.

Making contributions totally open to "quality code" will be a disaster.

Gene

[onekk]

Decisions about the 'value' of the contribution should be pushed upstream to the issue where it is entirely appropriate to discuss whether a problem is worth solving.

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?

Probably the problem is more "higher", and probably not taken in account by the C4 thing.

What is a PR, it is only a "bug fix" or it involve even "new functionalities".

If you have only "bug fixing" in mind the more correct way will be to raise the "Issue" and the "the fix" to explicitate the process, but if the process of raising the issue is more demanding than "fixing the bug" as it could be when "refactoring some part of code" by the "developer that has created the code" and these change won't have a visibile impact on the user experience and the "work of the program", think of moving as example "harcoded fonts" in a stylesheet.

This is not a "bug fix" and probably is not even an "issue" as the program is working, only the "procedure to change font appearance" could be cumbersome for some people.

But as example if this fix is done on a "limited part of the code" that involve maybe changing two lines of a 100 line function and refer to a centralize variable, and the developer has some spare time to make this thing?

It will be forced to submit an Issue maybe writing 50 lines of text or it would be better to use his time to change two line in the code so we all will have a better and simple interface.

I apologize for the "wall of word" and Regards

Carlo D.

[adrianinsaval]

Making contributions totally open to "quality code" will be a disaster.

C4's intention kinda goes the other way around, anything that is following procedure goes in whether it's quality code or not, so merging anything based on code quality was never the intention here or there.

Personal preferences, such as the recent flap about recoloring the main screen, also fall into the judgement category.

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, I don't think we need to adopt this though. But whether we like one proposed solution or the other I think there a problem was correctly identified.