On Wed, 24 Apr 2019 at 00:10, Gian Merlino <g...@apache.org> wrote: > Well, I think this is a good reason to have a robots-only policy for code > style checking. The fact that I am one of the few people that can adhere to > the unwritten style rules, because I've been working on Druid for over 7 > years, means that the system today doesn't work. >
What do you think about switching to a different, well-established code style? The problem with Druid's style is that neither IntelliJ, nor Eclipse, nor Checkstyle can fully support it at the moment, and it doesn't look to me that they will in a foreseeable future. If we choose, for example, Java Sun style or Google's style, I think at least Checkstyle should be able to fully cover it. I would add one thing: for all the "authors should proactively…" stuff, > reviewers should mention that when they leave comments. For example, > instead of "why did you do this?" a better review comment is "I can't > understand why you did this. Could you please add a code comment explaining > it?" Even better, if you suspect the code is wrong: "I can't understand why > you did this. It looks like it might be incorrect for X reason. Could you > please look into that, and if it seems like the code is correct after all, > add a comment explaining how it works?" > I agree that a reviewer should do this. It falls into the earlier point from my list: "Both the author and reviewers proactively make the life of other parties easier". However, the idea of that "proactive" list is that all parties should always take the most proactive action whenever they can regardless of whether the other party took a proactive action or not. Every reviewer can once just ask a question and forget to add that mouthful part about adding a comment, there will always be reviewers who won't follow this policy or won't be aware of it, etc. We want to avoid a situation when a reviewer just asks, the author just answers in a PR discussion, and knowledge is not recorded in the code. When both (regular) reviewers and authors do the proactive thing, this situation should be rare. > > It's a bit more work for reviewers but I think it makes a real difference > in the experience that authors have. It minimizes back-and-forth, which > minimizes frustration. And it helps propagate culture, since authors don't > just see what is being asked for, but also why, and they get a sense of > what the project's shared values are. > > > 3. Regularly review other people's PRs, don't just publish your code. > Gian > > have mentioned this. Addition from me: review PRs by people from not your > > org regularly. > > +1000 > > > 4. Regularly dedicate time to do a PR that increases automation of the > > project, for example: > > - Automate something about the CI or testing > > - Add a static analysis (IntelliJ) check > > - Add a Checkstyle check > > Anything we can move from manual review to robotic review is good. I would > caution against getting overly aggressive with static analysis & > checkstyle, but I don't think we're there yet. The checks we have don't > seem to be excessive. > > > 5. Regularly dedicate time to do a PR that is devoted specifically to > > repaying tech dept (yak shaving, refactoring) > > These are tough ones. Because all refactorings have risk, and because they > are not expected to yield bug fixes or functional improvements, they tend > to languish in the review queue. So extra work is needed by authors to > motivate review of them. I don't do too many of these sorts of PRs myself, > but when I do, I try to attach motivation. > I have the opposite opinion. 1) In my experience in Druid, a refactoring is usually associated with fixing old hidden bugs. Consider https://github.com/apache/incubator-druid/pull/7038, in which 5 different concurrency bugs were fixed. I'm sure that the majority of old concurrency classes in Druid have concurrency bugs, so doing any refactoring in this project: https://github.com/apache/incubator-druid/projects/4 will result in fixing bugs. 2) I think if we want to move to the good trajectory from https://martinfowler.com/bliki/DesignStaminaHypothesis.html, _reviewing_ refactoring PRs (nb: not doing them) should be a high priority. So not "extra work", but less work would be needed from authors to motivate reviewers to review it. Currently, there is a negative feedback cycle: people don't do refactoring because they know that it will take an infinite time to merge them, hence the associated conflict resolution work will be disproportionally huge. This is exacerbated by the fact that refactorings tend to touch more existing files in the codebase than feature PRs, and naturally, require more master conflict resolution work. Again: I don't say that we should refactor all the time, but I think that we want to ensure that when a developer decides to do some refactoring, he is confident that it will be relatively quick experience with little master conflict resolution work. > I don't think there is necessarily much value in this sort of refactoring, > if it's not making functional improvements or improving component design in > a way that makes long-term maintainability better. There may even be > negative value, since refactors take time to review and incur risk of > introducing new bugs. > I don't think people should usually refactor code that they never visited before, but whenever they sink 15 minutes..hours understanding (perhaps not for the first time) very ugly and unclear logic, they should definitely consider refactoring that logic during their next refactoring time slot. Regarding risks, I think it's usually the opposite: ugly code usually also contains bugs, and refactoring is an opportunity to find and fix them. I don't say there is no risk of introducing new bugs, but I think that the "expected bug delta in the codebase" in the result of refactoring is negative, not positive.
