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