Thanks, guys, I created https://issues.apache.org/jira/browse/HIVE-28072 I believe we all agree to introduce style checking to at least try to avoid further issues and how or whether to force it is a different question, feel free to follow HIVE-28072
Regards, Laszlo Bodor Zoltán Rátkai <zrat...@cloudera.com.invalid> ezt írta (időpont: 2024. jan. 8., H, 11:55): > +1 > > I think most of the devs use IntelliJ, where no other plugin needed to have > a Code Style. It can even import Eclipse formatting file. > > Regards, > > RZ > > On Mon, Jan 8, 2024 at 10:22 AM Stamatis Zampetakis <zabe...@gmail.com> > wrote: > > > +1 for enforcing style on new code. It will definitely save us from > > additional review cycles. > > > > Although I like checkstyle I tend to prefer tools that can > > automatically apply and fix style violations such as spotless [1]. > > > > It seems that the spotless plugin can be configured to enforce > > formatting gradually [2] so I think it is an ideal choice for this > > discussion. > > > > To avoid wasting CI resources for nothing we can employ spotless (or > > other plugins) during the regular build so that detect and fix style > > violations fail early on before raising the PR. > > > > Finally, spotless can be configured easily to apply Eclipse styles so > > making it use our recommended formatting [3] would be trivial. > > > > Best, > > Stamatis > > > > [1] https://github.com/diffplug/spotless > > [2] > > > https://github.com/diffplug/spotless/tree/main/plugin-maven#how-can-i-enforce-formatting-gradually-aka-ratchet > > [3] > > > https://github.com/apache/hive/blob/master/dev-support/eclipse-styles.xml > > > > On Mon, Jan 8, 2024 at 11:06 AM Zsolt Miskolczi > > <zsolt.miskol...@gmail.com> wrote: > > > > > > I think giving a warning is something that nobody will check. It could > > only > > > make sense if it is formatted in a way that it cannot be overseen. In > > every > > > other case, it is just ignored. And also, we are already full of > warnings > > > so I'm afraid it can just hide in the noise. > > > Sorry, I don't know how it works in hadoop/tez, maybe it is easy to > use. > > > > > > Ayush Saxena <ayush...@gmail.com> ezt írta (időpont: 2024. jan. 8., H, > > > 9:53): > > > > > > > +1, to have a checkstyle build. I am strongly against doing that big > > > > refactor to make just checkstyle happy, such a refactor will make > > > > backports to Hive lower branches tough and the life of folks > > > > maintaining downstream forks quite painful. > > > > > > > > We should enforce same kind of stuff like in Tez/Hadoop, where > > > > checkstyle violations are highlighted and the committer before > > > > committing can check that & decide whether that in unavoidable or not > > > > > > > > -Ayush > > > > > > > > On Mon, 8 Jan 2024 at 14:05, László Bodor <bodorlaszlo0...@gmail.com > > > > > > wrote: > > > > > > > > > > thanks for the responses so far! > > > > > I'm a bit against the one-time huge refactor commit as we don't > need > > that > > > > > (but I can be convinced of course), because checkstyle can be set > up > > to > > > > > warn only on style issues in the new/touched bits in the PR (or at > > least > > > > > that's how it works in tez), that's what we need, so we don't have > to > > > > make > > > > > that huge commit to simply introduce this enforcement > > > > > > > > > > Butao Zhang <butaozha...@163.com> ezt írta (időpont: 2024. jan. > 8., > > H, > > > > > 9:28): > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > > > BTW, We have a independent checkstyle file under iceberg module > > > > > > https://github.com/apache/hive/tree/master/iceberg/checkstyle . > I > > > > think > > > > > > we need to consider unifing the checkstyle in all the sub-module. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > Butao Zhang > > > > > > ---- Replied Message ---- > > > > > > | From | Zsolt Miskolczi<zsolt.miskol...@gmail.com> | > > > > > > | Date | 1/8/2024 16:19 | > > > > > > | To | <dev@hive.apache.org> | > > > > > > | Subject | Re: Force coding style in hive precommit | > > > > > > +1 > > > > > > > > > > > > In case there is an agreement about the coding style, we can > > prepare a > > > > tool > > > > > > that enforces that style at compile time. Run a tool one time to > > > > re-format > > > > > > all the existing code once. And turn on a compile time check. > > Iceberg > > > > did > > > > > > the same approach, they had one huge commit with almost 4k files > > > > changed > > > > > > and from that point, it worked well. And there are no issues > about > > > > > > formatting. > > > > > > I don't think putting a warning message helps at all. Also, it > > should > > > > be > > > > > > enforced on compile time. > > > > > > > > > > > > Zsolt > > > > > > > > > > > > Kirti Ruge <kirtirug...@gmail.com> ezt írta (időpont: 2024. jan. > > 8., > > > > H, > > > > > > 7:20): > > > > > > > > > > > > +1 > > > > > > As it would improve maintainability and code reviews. Sometimes > > small > > > > > > indentation/styling issues would kill review cycle time and we > can > > > > easily > > > > > > avoid it before requesting review. > > > > > > Enforcing more rules around it definitely boost guaranteeing > > quality. > > > > We > > > > > > can integrate it with git hooks. If we are going for this, I can > > work > > > > on > > > > > > getting it in place . > > > > > > > > > > > > Thanks, > > > > > > Kirti > > > > > > > > > > > > On 08-Jan-2024, at 11:36 AM, Akshat m <akshatats...@gmail.com> > > wrote: > > > > > > > > > > > > +1, We do have a documentation round it as well: > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/Hive/HowToContribute#HowToContribute-CodingConventions > > > > > > so it makes sense to enforce it as well. > > > > > > > > > > > > Right now we have a small section around this in documentation, > We > > can > > > > > > also > > > > > > expand this to a new page and add more Java practices to it as > well > > > > which > > > > > > are followed in the project while we are at this, Will be a great > > > > > > addition > > > > > > to Hive 4 documentation, I can pick it up. > > > > > > > > > > > > I suggest we add this style check as a pre-commit git hook as > > well, so > > > > it > > > > > > is enforced when the author is committing locally as well, this > can > > > > save > > > > > > the wait time for pre-commit failure in the PR for the author to > > > > realise > > > > > > the styling issues, ideally this should be taken care of with the > > ide > > > > > > style > > > > > > configuration but in case we miss it this would error out while > > > > > > committing the changes. > > > > > > > > > > > > Regards, > > > > > > Akshat > > > > > > > > > > > > On Sat, Jan 6, 2024 at 10:17 AM László Bodor < > > > > bodorlaszlo0...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > Hi All! > > > > > > > > > > > > What do you think about forcing coding style in Hive precommit? > > > > > > > > > > > > I remember, back in the old days, precommit printed some warnings > > in > > > > > > case > > > > > > some coding style (formatting, indentation, naming convention, > > etc.) > > > > > > problems were found in the patch, now it's simply not used, I > guess > > > > > > since > > > > > > we're using GitHub PRs. > > > > > > > > > > > > For example: I remember I simply approved a PR a few months ago > > which > > > > > > LGTM, and later just realized it's full of 4-spaces indentation, > > which > > > > > > is > > > > > > wrong if we assume that code should be formatted according to the > > style > > > > > > definition here: > > > > > > > > > > > > > > > > > > > https://github.com/apache/hive/blob/master/dev-support/eclipse-styles.xml > > > > > > > > > > > > I have just attached an example of Tez PR to open minds and > start a > > > > > > conversation. > > > > > > > > > > > > Regards, > > > > > > Laszlo Bodor > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
