richardstartin commented on pull request #8016: URL: https://github.com/apache/pinot/pull/8016#issuecomment-1014591577
> > > > I have two general points about the layout of the code (I don't like focussing on this sort of thing, but feel it's necessary here): > > > > > > > > 1. It needs to be formatted properly: https://github.com/apache/pinot/blob/master/config/codestyle-intellij.xml > > > > 2. There are too many blank lines. In my humble opinion, these do not aid the reader at all. On many occasions I look at 15 lines of code and notice they compress to 5. It really obscures the logic by occupying screen space. > > > > > > > > > Thanks for pointing that out. The problem is that my laptop is M1 Pro based, so a global build of Pinot fails on it. I am dependent on Linter checks to tell me if check style is failing. > > > > > > I would certainly prefer to automate style checking because I want code to be readable automatically without needing to review it for readability. If a simple method doesn't fit on a screen without scrolling, it's unreadable. I've proposed using checkstyle to ban blank lines within functions to @Jackie-Jiang to help specifically with this case, but he wasn't too keen on the idea and reasoned that the _occasional_ blank line can help readability. > > Regarding M1s, I think @dongxiaoman uses M1 and managed to build Pinot, maybe he can help? > > Thanks, I will check with @dongxiaoman. > > I would argue that the spacing around blanks is a subjective thing, unless somebody is putting 3 blanks consecutively (in which case, our check style does catch it). > > Beyond that, the definition of readability is YMMV. As long as it is not obviously and immediately evident that something is broken, we should IMO not let subjectivity take control This is not a subjective matter but a question of how much code fits on screen. Objectively, given finite screen space, large numbers of blank lines will prevent methods from fitting on screen at some point. One method in this PR contains 15 blank lines. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
