You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "attilakreiner (via GitHub)" <gi...@apache.org> on 2024/07/11 11:49:36 UTC
[PR] Update checkstyle definition [iceberg]
attilakreiner opened a new pull request, #10681:
URL: https://github.com/apache/iceberg/pull/10681
This is a follow-up PR for the discussion at #10673 where @findepi pointed out these two things w.r.t. the checkstyle definition:
- We could use possessive quantifiers in the regexps for performance reasons.
- Some custom error messages do not really have a lot of added value, so we'd better remove them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1673988656
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -480,23 +475,20 @@
<module name="LocalFinalVariableName"/> <!-- Java Style Guide: Local variable names -->
<module name="LocalVariableName"> <!-- Java Style Guide: Local variable names -->
<property name="tokens" value="VARIABLE_DEF"/>
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
<property name="allowOneCharVarInForLoop" value="true"/>
- <message key="name.invalidPattern" value="Local variable name ''{0}'' must match pattern ''{1}''."/>
</module>
<module name="MethodLength"/> <!-- Java Coding Guide: Methods and functions: focused, crisp, concise -->
<module name="MethodTypeParameterName"> <!-- Java Style Guide: Type variable names -->
- <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
- <message key="name.invalidPattern" value="Method type name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*+[T]$)"/>
Review Comment:
thanks. i also did check other changes. LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1673961429
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -480,23 +475,20 @@
<module name="LocalFinalVariableName"/> <!-- Java Style Guide: Local variable names -->
<module name="LocalVariableName"> <!-- Java Style Guide: Local variable names -->
<property name="tokens" value="VARIABLE_DEF"/>
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
<property name="allowOneCharVarInForLoop" value="true"/>
- <message key="name.invalidPattern" value="Local variable name ''{0}'' must match pattern ''{1}''."/>
</module>
<module name="MethodLength"/> <!-- Java Coding Guide: Methods and functions: focused, crisp, concise -->
<module name="MethodTypeParameterName"> <!-- Java Style Guide: Type variable names -->
- <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
- <message key="name.invalidPattern" value="Method type name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*+[T]$)"/>
Review Comment:
This one imo can't be possessive because of the trailing `T` requirement. For example `FooT` matched the previous pattern, but does not match the new pattern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "attilakreiner (via GitHub)" <gi...@apache.org>.
attilakreiner commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1673979787
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -480,23 +475,20 @@
<module name="LocalFinalVariableName"/> <!-- Java Style Guide: Local variable names -->
<module name="LocalVariableName"> <!-- Java Style Guide: Local variable names -->
<property name="tokens" value="VARIABLE_DEF"/>
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
<property name="allowOneCharVarInForLoop" value="true"/>
- <message key="name.invalidPattern" value="Local variable name ''{0}'' must match pattern ''{1}''."/>
</module>
<module name="MethodLength"/> <!-- Java Coding Guide: Methods and functions: focused, crisp, concise -->
<module name="MethodTypeParameterName"> <!-- Java Style Guide: Type variable names -->
- <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
- <message key="name.invalidPattern" value="Method type name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*+[T]$)"/>
Review Comment:
You're right, thx for point it out. I reverted this one change. I double checked the other changes for similar issues and they LGTM. Pls chk and LMK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1678312407
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -281,16 +280,13 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
- <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
Review Comment:
Could you explain what the ++ syntax means?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #10681:
URL: https://github.com/apache/iceberg/pull/10681
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "attilakreiner (via GitHub)" <gi...@apache.org>.
attilakreiner commented on PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#issuecomment-2223052922
Merged in the `main` branch and fixed `ConstantName`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1678370083
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -281,16 +280,13 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
- <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
Review Comment:
Well checkstyle currently takes less than 1 second on my machine :) so I'm not super worried about the perf. In reading your article it does seem like this is a situation where a good regex engine wouldn't backtrack anyway because $ is incompatible with the previous matching class. Is Java a good regex engine? I don't know.
Anyway, I have no problem with merging this since it is semantically the same for our purposes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "attilakreiner (via GitHub)" <gi...@apache.org>.
attilakreiner commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1678355098
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -281,16 +280,13 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
- <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
Review Comment:
The `*+` and `++` are _possessive_ quantifiers (as opposed to `*` and `+` which are _greedy_; and `*?` and `+?` which are `lazy`).
I found the explanation on this page very useful: https://www.rexegg.com/regex-quantifiers.php#possessive
My understanding is that in cases when we can use both _greedy_ and _possessive_ quantifiers, _possessive_ are faster. Quote form the link above:
> This behavior is particularly useful when you know there is no valid reason why the engine should ever backtrack into a section of matched text, as you can save the engine a lot of needless work.
This seems a common regex feature, but honestly speaking I wasn't familiar with in until @findepi pointed it out (thx for that) in the review of #10673.
I understand using _possessive_ quantifiers is good for performance, OTOH it's possible that many people are unfamiliar with this syntax, so it may be a tradeoff in terms of readability (and also a deviation from checkstyle standards). WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "attilakreiner (via GitHub)" <gi...@apache.org>.
attilakreiner commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1678355098
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -281,16 +280,13 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
- <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
Review Comment:
The `*+` and `++` are _possessive_ quantifiers (as opposed to `*` and `+` which are _greedy_; and `*?` and `+?` which are _lazy_).
I found the explanation on this page very useful: https://www.rexegg.com/regex-quantifiers.php#possessive
My understanding is that in cases when we can use either _greedy_ and _possessive_ quantifiers, _possessive_ are faster. Quote form the link above:
> This behavior is particularly useful when you know there is no valid reason why the engine should ever backtrack into a section of matched text, as you can save the engine a lot of needless work.
This seems a common regex feature, but honestly speaking I wasn't familiar with in until @findepi pointed it out (thx for that) in the review of #10673.
I understand using _possessive_ quantifiers is good for performance, OTOH it's possible that many people are unfamiliar with this syntax, so it may be a tradeoff in terms of readability (and also a deviation from checkstyle standards). WDYT?
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -281,16 +280,13 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
- <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
Review Comment:
The `*+` and `++` are _possessive_ quantifiers (as opposed to `*` and `+` which are _greedy_; and `*?` and `+?` which are _lazy_).
I found the explanation on this page very useful: https://www.rexegg.com/regex-quantifiers.php#possessive
My understanding is that in cases when we can use either _greedy_ or _possessive_ quantifiers, _possessive_ are faster. Quote form the link above:
> This behavior is particularly useful when you know there is no valid reason why the engine should ever backtrack into a section of matched text, as you can save the engine a lot of needless work.
This seems a common regex feature, but honestly speaking I wasn't familiar with in until @findepi pointed it out (thx for that) in the review of #10673.
I understand using _possessive_ quantifiers is good for performance, OTOH it's possible that many people are unfamiliar with this syntax, so it may be a tradeoff in terms of readability (and also a deviation from checkstyle standards). WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "attilakreiner (via GitHub)" <gi...@apache.org>.
attilakreiner commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1678355098
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -281,16 +280,13 @@
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="MemberName"> <!-- Java Style Guide: Non-constant field names -->
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
- <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
Review Comment:
The `*+` and `++` are _possessive_ quantifiers (as opposed to `*` and `+` which are _greedy_; and `*?` and `+?` which are _lazy_).
I found the explanation on this page very useful: https://www.rexegg.com/regex-quantifiers.php#possessive
My understanding is that in cases when we can use both _greedy_ and _possessive_ quantifiers, _possessive_ are faster. Quote form the link above:
> This behavior is particularly useful when you know there is no valid reason why the engine should ever backtrack into a section of matched text, as you can save the engine a lot of needless work.
This seems a common regex feature, but honestly speaking I wasn't familiar with in until @findepi pointed it out (thx for that) in the review of #10673.
I understand using _possessive_ quantifiers is good for performance, OTOH it's possible that many people are unfamiliar with this syntax, so it may be a tradeoff in terms of readability (and also a deviation from checkstyle standards). WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "attilakreiner (via GitHub)" <gi...@apache.org>.
attilakreiner commented on code in PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#discussion_r1673979787
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -480,23 +475,20 @@
<module name="LocalFinalVariableName"/> <!-- Java Style Guide: Local variable names -->
<module name="LocalVariableName"> <!-- Java Style Guide: Local variable names -->
<property name="tokens" value="VARIABLE_DEF"/>
- <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
+ <property name="format" value="^[a-z][a-zA-Z0-9]++$"/>
<property name="allowOneCharVarInForLoop" value="true"/>
- <message key="name.invalidPattern" value="Local variable name ''{0}'' must match pattern ''{1}''."/>
</module>
<module name="MethodLength"/> <!-- Java Coding Guide: Methods and functions: focused, crisp, concise -->
<module name="MethodTypeParameterName"> <!-- Java Style Guide: Type variable names -->
- <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
- <message key="name.invalidPattern" value="Method type name ''{0}'' must match pattern ''{1}''."/>
+ <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*+[T]$)"/>
Review Comment:
You're right, thx for pointing it out. I reverted this one change. I double checked the other changes for similar issues and they LGTM. Pls chk and LMK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Update checkstyle definition [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #10681:
URL: https://github.com/apache/iceberg/pull/10681#issuecomment-2248879112
Thanks @attilakreiner for the commit and @findepi for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org