+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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
