You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2024/08/06 13:30:15 UTC
[PR] Build: Add checkstyle rule to ban assert usage [iceberg]
nastra opened a new pull request, #10886:
URL: https://github.com/apache/iceberg/pull/10886
(no comment)
--
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] Build: Add checkstyle rule to ban assert usage [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #10886:
URL: https://github.com/apache/iceberg/pull/10886#discussion_r1705578042
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -414,6 +414,11 @@
<property name="format" value="@Test\(.*expected.*\)"/>
<property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="^\s*assert\s+"/>
Review Comment:
can you give https://checkstyle.sourceforge.io/checks/coding/illegaltoken.html#IllegalToken with LITERAL_ASSERT token a try?
--
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] Build: Add checkstyle rule to ban assert usage [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10886:
URL: https://github.com/apache/iceberg/pull/10886#discussion_r1705585282
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -414,6 +414,11 @@
<property name="format" value="@Test\(.*expected.*\)"/>
<property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="^\s*assert\s+"/>
Review Comment:
I don't think we want to match code that doesn't compile or isn't properly formatted. `fooassert´ isn't a valid statement and `assert(true)` would be flagged as not properly formatted code by spotless, thus resulting in `assert (true)`.
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -414,6 +414,11 @@
<property name="format" value="@Test\(.*expected.*\)"/>
<property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="^\s*assert\s+"/>
Review Comment:
I don't think we want to match code that doesn't compile or isn't properly formatted. `fooassert` isn't a valid statement and `assert(true)` would be flagged as not properly formatted code by spotless, thus resulting in `assert (true)`.
--
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] Build: Add checkstyle rule to ban assert usage [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #10886:
URL: https://github.com/apache/iceberg/pull/10886#discussion_r1705567611
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -414,6 +414,11 @@
<property name="format" value="@Test\(.*expected.*\)"/>
<property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="^\s*assert\s+"/>
Review Comment:
current pattern needs some improvement, since it also matches `fooassert` (shouldn't) and doesn't match `assert(true);` (should).
instead of `\s*` / `\s+`, check it's a word boundary: `\b` (`\bassert\b`)
I thought there should be a dedicated way to do this, without having to use regular expressions.
in fact, error-prone claims to do this: https://errorprone.info/bugpattern/UseCorrectAssertInTests . why it didn't work for us?
--
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] Build: Add checkstyle rule to ban assert usage [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10886:
URL: https://github.com/apache/iceberg/pull/10886
--
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] Build: Add checkstyle rule to ban assert usage [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10886:
URL: https://github.com/apache/iceberg/pull/10886#discussion_r1705611373
##########
.baseline/checkstyle/checkstyle.xml:
##########
@@ -414,6 +414,11 @@
<property name="format" value="@Test\(.*expected.*\)"/>
<property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="^\s*assert\s+"/>
Review Comment:
@findepi `LITERAL_ASSERT` does the trick and is much better than regex matching, thanks
--
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] Build: Add checkstyle rule to ban assert usage [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #10886:
URL: https://github.com/apache/iceberg/pull/10886#issuecomment-2271669505
thanks for the reviews @findepi @bryanck @amogh-jahagirdar
--
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