I can agree I am not the faster merger here. That one is on me. I have limited time.
No, I am not "picky" with the code. You may ask other contributors. When things are within the contribution guidelines, merges come reasonably fast (at least when I am not sick or worse, like last summer). Even highly transcendental changes of mathematically demanding code (such as Ajinkya's B-Spline solver support PRs) have been merged in about two months (including improvement cycles). I try to care about code maintainability and I put an emphasis in software architecture, because I know the consequences of not doing it. I put a lot of thought into backward and forward compatibility, because I think the Sketcher is one of the most essential parts of FreeCAD, if not the most, and problem in the sketcher would have a very strong impact in FreeCAD as a whole.
Paddle's PRs have had tons of problems that, if merged as is, would have made future maintainability a nightmare. No, it is not a matter of "preference". That is very unfair, specially taking into account the amount of hours I have invested in reviewing code, exchanging PMs explaining problems and providing solutions. I have explained several times the reasons why software architecture is important. Other argument I needed to hear is that other parts of the Sketcher are not that shiny, so I was being unfair to him because, that was his position, I was demanding more of him than of others. Even if it were true that parts of the Sketcher should be refactored, why on earth should that be an argument to merge code with insufficient quality? The solution is to fix the issues so that the code is acceptable, not to try to push it as is.
I can agree Paddle has been consistent in disregarding software architecture. As he has claimed several times, he does not deem it important. After all, he appears to believe, it is the task of the maintainers to invest substantial effort in fixing his PRs or deal with the consequences of merging them. Last year most of my FC investment has gone into him. It is not a complaint. I considered it an investment. I hoped I would be able to convince him to do things differently. I have not managed to. Still it was my choice and I feel no regrets. I would do it again.
However, my decision to invest all my FC time on him did have consequences. Ajinkya was the most negatively impacted by my decision. Some of his simpler B-Spline PRs were on the pipeline for months, as I did not have time left for him first, then I got sick and when I recovered, there were more Paddle PRs waiting. And that is a real pity indeed, because Ajinkya's PR's were closer to mergable. I was really unfair to him (and I am sorry). Yet Ajinkya did not complain (even though he had every reason to). Conversely, Paddle complained once and again. I was closed to a burnout end of the last year. I think I was not the only maintainer.
So Paddle is abhorrent? No, not at all. Paddle is brilliant in many ways. He has an eye about how to make meaningful changes. He is fast coding and can find his way through code complexity. He is very resourceful in hacking away code. Unfortunately, part of the code is such a hack that needs rewriting. I wanted to arrive to the point were he would realise this and fix his PRs before submitting them. To me he has a bad balance of productivity and quality, but with a huge potential of improvement.
Now, this may counter some positions, but brings us no closer to having code and features merged.
adrianinsaval wrote: ↑Fri Jan 27, 2023 2:09 pm
it also keeps new devs away making it harder to sustain the project and maintain the code so benefits are somewhat questionable, we had discussed and agreed on a contribution process, but sadly none of it was followed for these cases, neither from the contributor nor from the maintainers. Now many good features are in the limbo with a developer that has lost motivation due to failure of the process.
There is one thing we do need to understand: It is harder to sustain the project if new devs' PRs are unmergeable or require substantial changes, than with lesser devs. This is not a call for lesser devs. I fo prefer more devs. However, it is a call for balance. We gain nothing if our maintainers end up with a burnout.
I understand that the contribution process has a potential to be used to shame the maintainer and blame him or her (why don't we have female maintainers here? Must be the contribution process...). Almost everything can be argued to be "preferences". Here I would like to warn against weaponising the contribution process. Yes, maintainers need to be held accountable. But code quality is not a "preference". No, a maintainer is not required to produce a counter PR if quality is not ok. He has a substantial amount of discretion (not arbitrariness), which is necessary to do his work.
However, to be fair to everybody, including Paddle, we discussed a contribution process way after most of Paddle's PRs. His latest PRs are way better than the earlier ones. The ones that could be fixed without substantial time investments were merged. The ones that have issues need those issues addressed. I said I would do that after I PR the NotificationArea (I have the time I have, and no more). If all the PRs were of acceptable quality, much would be already merged, it not all.
Anyway, it is not just the process that defines the outcome. The quality of the code is the most important factor. A huge PR can be merged if the quality is such that no objection can be raised. However, huge PRs having lots of problems have low chances and cost a lot of iterations, time and effort of the maintainers. Similarly, it has happened that, as a result of asking a change, we got several other unasked changes which forced to start the review anew. That is also very demotivating for maintainers. The process is a means to try to avoid to reach those cases. I do not think "the process failed". The process was not conceived to treat the cases that failed. The persons failed in not adhering to the process, or failing to produce code of sufficient quality. The process does not write the code.
Am I happy that Paddle lost his motivation? Not at all. Is there anything I could have done beyond what I did? I do not think so. Are there good ideas in the limbo? Yes, there are. Are they lost? No, they aren't. Will they be merged tomorrow? Not really in their present form. Then when? When they morph into an acceptable form.
Hologram wrote: ↑Fri Jan 27, 2023 2:38 pm
Ideally, both sides will be kept satisfied. That'll mean taking the middle ground; adopting patches relatively easily, yet with maintainable code. Easier said than done, I know.
But from a user point of view @paddle created things that even Fusion 360 struggles with. That's really nice and a way to stand out from the crowd. These are really the selling, exiting features for end users.
Yes. Paddle is a very intelligent individual and has very good ideas. He has also improved his coding skills over the last year and his knowledge of FreeCAD. He has a lot of potential to produce great features. He tries to push his boundaries, which is great for his learning. However, often he ends up with more code complexity than he can manage, which leads to code problems. He tends to try to overachieve in PRs, which results in an accumulation of code problems. He can indeed produce very shiny features. He would need to put more emphasis into code quality and code architecture. Here, it is a major hurdle that he does not believe in that need.
As I see it, we come from an almost burnout. There is a need to revisit Paddle PRs. Time will tell how everybody finds his way in the project. That is what is anyway what I can offer ATM.