You implied it, because precisely sketcher offset is part of a extremely big PR that never ended growing. Reality is seldom simple. As said, I do not regret the time invested. I would do it again for you or for any other showing interest in the sketcher.
There are different types of PRs:paddle wrote: ↑Mon Jan 30, 2023 10:01 am But yes, I still say that you guys are too picky on code architecture, because even when the PR is acceptable, if it doesn't match what you deam the exact optimal solution then it doesn't merge.
Look at the grid PR. I first do the job. I'm told it's not good because it's not in a coinmanager and bloats the VPsketch. I do it and what? I'm told again that this is not good.
Besides, having the grid drawn by a coinmanager as every other things in sketcher does makes sense. More so than having a weird inheritance of VPsketch. So you can't say that the PR in it's current state has an unacceptable architecture. It's just not matching your opinion of optimal.
- There are PRs which touch a very well defined area. As far as the class interface is ok and code is not specially ugly, I can go along with it if it works.
- There are PRs that spread over quite some extent. Here the structure of the code needs to be sound. Not perfect, just sound. I needs not to force maintainers to have to rewrite it.
- There are PRs that touch a complete hierarchy of classes over different WBs. Here architecture is a must. There are no buts. You may call me names.
You have a taste for the later two types. Complexity needs to be managed on those types of PRs.
Now, you come to the grid PR. First you do "the job". I can agree it was shiny on the outside. Then, you took out of context an indication from me that if the PR was going to touch Coin, there should be something similar to a coinmanager. Later, I reviewed part of the code and found that a lot of Grid related properties had been added to ViewProviderSketch. I stopped reviewing and asked for an opinion from Werner. I was never convinced that these properties should be there. At the end, Werner's opinion is there for anybody to read. To some extend, it confirms that is not ok (a breach of the single responsibility principle, SRP).
So, what you call "a weird inheritance of VPsketch" is actually a breach of the SRP. The SRP is a real thing, you may want to Google it... or wait, probably you can go back to our many discussions. One class should have one responsibility. Sketcher's coinManager responsibility is about representing sketcher features. The grid is not a sketcher feature, it is just a grid. It is even optional.
You may have a point that I could have done things differently (better communication). However, that would have required even more time when I was close to a burnout from may other PRs. You can blame me on that. I will take it.
The latest sentence embodies one of your biggest shortcomings: you just do not care about Architecture. It is not that you cannot learn about it. You are more than smart enough. You just do not want to learn about it because for you it is complexity, rather than a way to handle complexity and preventing code to grow out of control (which is what it actually is). I failed to help you understand that. I tried, but did not succeed.
You produce shiny features, but you minimise the investment beyond the appearance. Maintainers need you to invest more time into producing good quality code, so that features can be merged with a small number of iterations or none.
Again, this is the wrong approach to it. I am not merging code just because code is there waiting to be merged. Any code cannot be merged. If we were 10 people full time, somebody would have fixed the PR already and merged it. Reality is different. The consequence is that certain PRs need to wait.paddle wrote: ↑Mon Jan 30, 2023 10:01 am If you were 10 people full time with the bandwidth to go the extra-lenght to spend 20 hours rewriting the grid PR to this optimal version, why not. But as it stands maintainers don't have the bandwidth to do so. So yes I don't think that being this picky is worth it.
If there is any blame to be attributed, I should be the recipient. I do not accept that OpenBrain is to any kind of blame. If anything, I should have supported OpenBrain better, as he got a very difficult assignment. Not all PRs are the same. Beyond that, OpenBrain has helped Paddle a lot over time. He has reviewed or helped me review other PRs of Paddle. OpenBrain has coded snippets for Paddle to help him fix issues. I have no doubt his intentions were good.adrianinsaval wrote: ↑Mon Jan 30, 2023 12:43 pm I will support paddle here on the case of the grid PR (which I followed closely), but I don't think this is a generalized problem, this was being handled by a very new maintainer (openBrain) not abdullah, so for starters I think it's unfair to put all this on abdullah. I also don't think we need to crucify openBrain or the entire maintainers team over this, he made a mistake (which is to be expected from anyone starting on something new) and I hope he learns from it but I believe his intentions were good.
But we must at least recognize that this was very unfair to paddle as he was made to reestructure his PR unnecessarily and into an architecture that was then deemed incorrect by other maintainers. I think an apology and "let's all try to do better next time" is what is needed.
The fact that Paddle moved code to a coinmanager is not per se wrong. The problem is that that coinmanager should be in a separate class dealing with the Grid (either a ViewProvider or a ViewProviderExtension, the latter is Werner's pick). Properties should not be moved to ViewProviderSketch. When ViewProviderSketch inherings from another ViewProvider or a ViewProviderExtension, those properties "are" part of ViewProviderSketch, but are not defined in ViewProviderSketch. This allows to achieve two goals: (a) all the grid functionality is in one separate class/extension, so it can be leveraged by any other ViewProvider in the future, (b) it prevents unnecessary growing of ViewProviderSketch, which means more code to maintain and poor separation of responsibilities (because without having to conform to an interface between objects, developers end up inter-tangling different responsibilities making it very tedious to untangle them later on). I spend around two months untangling ViewProviderSketch one year ago (and I did not untangle everything I wanted to).
That said, the decision to move code to a coinmanager was not a bad decision. It is just unfinished. It is just one more thing that needed to be tackled in that PR (and one can read the whole story, as it is available in GH, there were many things tackled).