Re: Force coding style in hive precommit
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 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 > 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 > > 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 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 > > > > > 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 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 | > > > > > > | Date | 1/8/2024 16:19 | > > > > > > | To | | > > > > > > | Subject | Re: Force coding style in hive precommit | > > > > > > +1 > > > > > > > > > > > > In case ther
Re: Force coding style in hive precommit
+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 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 > 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 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 > > > 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 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 | > > > > > | Date | 1/8/2024 16:19 | > > > > > | To | | > > > > > | 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 ezt írta (időpont: 2024. jan. > 8., &
Re: Force coding style in hive precommit
+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 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 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 > > 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 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 | > > > > | Date | 1/8/2024 16:19 | > > > > | To | | > > > > | 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 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 wrote: > > &g
Re: Force coding style in hive precommit
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 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 > 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 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 | > > > | Date | 1/8/2024 16:19 | > > > | To | | > > > | 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 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 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 > > > > > > > > > > > > > > > >
Re: Force coding style in hive precommit
+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 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 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 | > > | Date | 1/8/2024 16:19 | > > | To | | > > | 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 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 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 > > 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 > > > > > > > > > >
Re: Force coding style in hive precommit
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 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 | > | Date | 1/8/2024 16:19 | > | To | | > | 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 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 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 > 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 > > > > >
Re: Force coding style in hive precommit
+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 | | Date | 1/8/2024 16:19 | | To | | | 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 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 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 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
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 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 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 > > 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 > >> > >> > >
Re: Force coding style in hive precommit
+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 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 > 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 >> >>
Re: Force coding style in hive precommit
+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 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 > >
Force coding style in hive precommit
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
