Devs - We COULD have a more rigorous Review of code, prior to accepting a Commit (RTC), but this may slow down development.
That said, there are some strong arguments for improving the quality of the PRs and Commits. This is, after all, a financial services software. Some back and forth on this to share with the full community below, and seeking more discussion, especially from those who are actively committing. Nothing has been decided. So, Do we need to move to a more thorough RTC (review, then commit)? Is it sufficient for one person to review with a LGTM (looks good to me)? How do we ensure we follow our code review standards and suggested practices? (https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide) ---- From Jdailey --- At ApacheCON I met with a longstanding Apache member and discussed some of the issues we've seen on the project. I think some of the code quality issues stem from JIRA issue tickets not being fully developed, and then PRs being put through and allowed to be committed without proper and full review before or after. Our fineract wiki page - which I edited last in 2018 says: "To support the committer's process, Fineract is adopting a lazy consensus for changes. We trust Committers will use their powers for good and not break the code." https://cwiki.apache.org/confluence/display/FINERACT/Committer%27s+Zone The process is essentially a "soft" Review then Commit (RTC), but there is trust that the dev finds another *Fineract Committer* to review, and the assumption that 72 hrs is enough time from someone else to review and object if necessary. It assumes good intent, which is nice. On the other hand, I believe that I am actually seeing a pattern of mostly Commit then Review, at least for some code changes, which in practice means that the release manager is looking through all of the Commits when they are trying to make a release. @Aleksandar Vidakovic - can you verify this is what you are doing? So, how do we encourage more of the ethic of review? and better and better quality code? Do we need to go back to some formal voting of Commits? i.e. RTC whereby at least three Committers must agree on the change? And where a -1 is a veto? --- from Greg Stein, Apache Member ---- * dev process "should" be discussed by the entire community on the dev@ list, rather than privately by a subset of the community * I've found over the past couple decades that (1) review is sufficient (ie. 3 reviews will grind the project to a halt). In the old (svn) days, that would be a patch to the mailing list and a single "+1" or "lgtm" (looks good to me) and the sender could then apply their patch. In GitHub days, I'd suggest one person post a PR, and that a reviewer does the merge. That gives you a record of two people approving the change. No self-merges except for "Obvious Fixes" (eg. typos, changes to comments/doc/readme/websites (not-code)). --- from Aleks ---- .. I think what's mostly missing is a set of good practices that everyone should have on the radar. We do have all those linters (Spotless) in place, but there are some "patterns" (e. g. how to name classes and packages; where to place them) that can slip through the cracks. Here's a list of things that recently appeared on my radar (re-post from Slack): - how to edit Asciidoc files (don't bother adding your own line breaks) - sometimes it's not clear where to put certain classes (DTOs, entity classes, REST resource classes, Spring Java configuration, Spring configuration validation, exceptions, constants, de-/serializers, mappers, handlers, tasks, jobs) - nested folders: we already have quite some levels of folders/packages until we actually write code (e. g. "org.apache.fineract.portfolio.client.service" etc.); I would not recommend doing more folders beyond the "usual" ones; e. g. I would not create something like "org.apache.fineract.portfolio.client.service.service2.Service1Impl" and "org.apache.fineract.portfolio.client.service.service1.Service2Impl", but just make sure the classes are unique and put them on one level ("org.apache.fineract.portfolio.client.service.Service1Impl", "org.apache.fineract.portfolio.client.service.Service2Impl" - sometimes we have utility classes that are only used in one place (e. g. for Spring configuration); consider embedding them instead of creating separate files; this makes it easier for devs to differentiate important stuff from auxiliary structures - fix compile warnings as much as possible; we used to have actually only 1-2 warnings in fineract-client, because of the way how the code generator works; but recently we got 20-30, mostly related to generic types; we used to have a very strict compiler setting that treated those warnings like errors (would fail the build) - would be great if all contributors could configure their Github account with a proper GPG key to get verified tag associated to their commits FYI: list above has no specific order... no claim that it's complete or that all items are equally important to everyone... but having some of those rules in place would make efforts to reduce boilerplate code and move towards real modules certainly easier. ---
