You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/15 22:08:48 UTC
[GitHub] [iceberg] xrl1 opened a new pull request, #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
xrl1 opened a new pull request, #5057:
URL: https://github.com/apache/iceberg/pull/5057
Closes #4618
From the issue:
`Preconditions.checkArgument` only works with %s, as %d does not parse and returns a bad error message.
Created a multiline regex to prevent this.
The regex is as follows:
```regex
Preconditions\.checkArgument\([^;]+,\s+"[^;]*%d[^;]+\);
```
The logic behind it: Find the function call, then some first parameter until a ',' (as the error message is the second parameter), and finally search until the function closes and a ';'.
* Used `[^;]*` instead of `.*`, because this is a multiline search and the dotall regex param (enabled by `matchAcrossLines` config) is aggressive and this is the best way to break its search.
* It can be removed and the "\s+" can be enough
* Added the quote and comma to avoid matching the first argument, but I can simplify the regex and not handle the edge cases where there is "%d" in the first argument
Tell me what you think, I have no problem tuning it
Also, fixed %d usages that the Checkstyle rule found
--
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
[GitHub] [iceberg] szehon-ho commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1167772854
Thanks @xrl1 and @kbendick for additional 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
[GitHub] [iceberg] kbendick commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1167647041
> @szehon-ho Yeah you are right, it is really unlikely to encounter `%d` in the first argument I simplified it to:
>
> ```regexp
> Preconditions\.checkArgument\([^;]+%d[^;]+\);
> ```
Could you update the PR description with the explanation of this regex (so we don't have to look it up in the conversation) @xrl1? I believe the original regex is still there.
--
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
[GitHub] [iceberg] xrl1 commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
xrl1 commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1164153523
@szehon-ho I'll explain a little bit more:
As requested, I used a multi-line regex search as asked, and I set `matchAcrossLines` to `true` to enable "DOTALL flag" as we don't know on which line the "%d" expression will appear after the function call.
Once this value is set, if I insert `.*` before the `%d` in the expression (to match the first argument to `checkArgument`, and not only to skip `%d` in it as you asked), it will search for matches for the second part of the regex, in all the file, even after checkArugment statement.
For example:
```
Preconditions.checkArgument(partitionFields.size() >= 1, "some text %s", 1);
someOtherFunc("%d", 100);
```
might be mistakenly matched.
To avoid matching it, the `[^;]` will terminate the search upon the first `;` it encounters, and problem solved.
--
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
[GitHub] [iceberg] szehon-ho commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1167634406
Great, thanks for spending time on this. Will merge once it passes
--
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
[GitHub] [iceberg] szehon-ho commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1162498407
Looks like some violation in Spark 3.0? Also fyi , @kbendick
--
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
[GitHub] [iceberg] szehon-ho commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1163781495
Sounds good, sorry i am pretty bad at regex, what do you mean:
> Used [^;]* instead of .*, because this is a multiline search and the dotall regex param (enabled by matchAcrossLines config) is aggressive and this is the best way to break its search.
> It can be removed and the "\s+" can be enough
The [^;]* can be removed? The purpose now is to not match %d in the Precondition.checkArgument()'s first argument (or is it for something else)?
--
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
[GitHub] [iceberg] xrl1 commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
xrl1 commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1166283589
@szehon-ho Yeah you are right, it is really unlikely to encounter `%d` in the first argument;
simplified it to:
```regex
Preconditions\.checkArgument\([^;]+%d[^;]+\);
```
--
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
[GitHub] [iceberg] xrl1 commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
xrl1 commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1164155161
Updated the PR again, and now also tested the build locally to pass, sorry for that.
--
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
[GitHub] [iceberg] szehon-ho commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1164925668
@xrl1 thanks for the explanation for those points. How about the last point:
> Added the quote and comma to avoid matching the first argument, but I can simplify the regex and not handle the edge cases where there is "%d" in the first argument
Do you think we should simplify this? I don't see much possibility of %d in the first argument of Precondition (which is evaluating a boolean), other than a modulo operation but in that case there is a space after %. Or let me know if I missed what you mean.
--
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
[GitHub] [iceberg] xrl1 commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
xrl1 commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1162750225
@szehon-ho I committed a fixup to the violation (missed it while running the checkstyle locally in the IDE for some reason)
--
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
[GitHub] [iceberg] xrl1 commented on pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
xrl1 commented on PR #5057:
URL: https://github.com/apache/iceberg/pull/5057#issuecomment-1167668381
@kbendick Yeah sure, updated.
--
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
[GitHub] [iceberg] szehon-ho merged pull request #5057: Checkstyle: Add checkstyle rule for %d in Preconditions.checkArgument
Posted by GitBox <gi...@apache.org>.
szehon-ho merged PR #5057:
URL: https://github.com/apache/iceberg/pull/5057
--
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