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