You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "timtebeek (via GitHub)" <gi...@apache.org> on 2023/04/19 11:26:41 UTC
[GitHub] [maven-pmd-plugin] timtebeek opened a new pull request, #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
timtebeek opened a new pull request, #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124
### [Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)](https://public.moderne.io/recipes/org.openrewrite.java.migrate.apache.commons.lang.IsNotEmptyToJdk)
Followup from #120 for @elharo
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] elharo commented on pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#issuecomment-1516148340
The "co-authored" message has the potential to cause licensing and ownership issues. Alternately, Moderne could assign the corporate CLA: https://www.apache.org/licenses/cla-corporate.pdf
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] timtebeek commented on a diff in pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "timtebeek (via GitHub)" <gi...@apache.org>.
timtebeek commented on code in PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#discussion_r1171202445
##########
src/main/java/org/apache/maven/plugins/pmd/PmdViolationCheckMojo.java:
##########
@@ -143,7 +142,7 @@ protected ViolationDetails<Violation> newViolationDetailsInstance() {
private String getFilename(String fullpath, String pkg) {
int index = fullpath.lastIndexOf(File.separatorChar);
- while (StringUtils.isNotEmpty(pkg)) {
+ while ((pkg != null && !pkg.isEmpty())) {
Review Comment:
```suggestion
while (pkg != null && !pkg.isEmpty()) {
```
##########
src/main/java/org/apache/maven/plugins/pmd/ExcludeViolationsFromFile.java:
##########
@@ -107,9 +106,9 @@ private boolean isExcludedFromFailure(String className, String ruleName) {
private String extractClassName(String packageName, String className, String fullPath) {
// for some reason, some violations don't contain the package name, so we have to guess the full class name
// this looks like a bug in PMD - at least for UnusedImport rule.
- if (StringUtils.isNotEmpty(packageName) && StringUtils.isNotEmpty(className)) {
+ if ((packageName != null && !packageName.isEmpty()) && (className != null && !className.isEmpty())) {
Review Comment:
```suggestion
if (packageName != null && !packageName.isEmpty() && className != null && !className.isEmpty()) {
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] timtebeek commented on pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "timtebeek (via GitHub)" <gi...@apache.org>.
timtebeek commented on PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#issuecomment-1514784283
Thanks for the quick review. It's now very easy to create more of these pull requests, but I obviously want to limit spam and only make these if welcome. Any guidance there on how you'd like to proceed?
I'd imagine we can fairly easily remove 1000+ instances where we use is(Not)Empty, after which we can look at the next top item to replace is, to reduce our dependency on StringUtils (whichever dependency that comes from). I imagine you'd want to tackle each in separate batches of pull requests right? To keep review easiest.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] elharo merged pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo merged PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] elharo commented on pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#issuecomment-1516150217
In fact, now that I think about it, we should pause these changes until Moderne does sign the corporate CLA. An Individual CLA is insufficient given the co-authorship line.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] timtebeek commented on pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "timtebeek (via GitHub)" <gi...@apache.org>.
timtebeek commented on PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#issuecomment-1539062036
> In fact, now that I think about it, we should pause these changes until Moderne does sign the corporate CLA. An Individual CLA is insufficient given the co-authorship line.
@elharo I'm pleased to say we've finally been able to sign (and get accepted) the CCLA:
> This message acknowledges receipt of the following document, which has been filed in the Apache Software Foundation records:
> CCLA from Moderne Inc.
Would you want me to resume creating pull requests to replace `StringUtils#is/Not/Empty(String)` as we've done here?
I was thinking to open up to ten at a time, and only start a next batch when there's less than five open; does that sound OK?
The changes would be made by running this recipe against the Apache Maven organization defined in the top right corner:
https://public.moderne.io/recipes/org.openrewrite.java.migrate.apache.commons.lang.IsNotEmptyToJdk
You're more than welcome to have a look as well; shouldn't take more than a couple minutes to create a few PRs or direct commits.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] timtebeek commented on pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "timtebeek (via GitHub)" <gi...@apache.org>.
timtebeek commented on PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#issuecomment-1515932350
> You can assign these reviews to me. I might not get to all of them this quickly but they're not hard to review.
Appreciate it! I'll start with the project that benefit the most; then work my way down to the long tail of incidental usage, as soon as we check off the following item:
> I would like it if you can amend the commit messages so they no longer say "Co-authored-by: Moderne".
The only way I can change this now would be to manually amend the commits, which I'd like to avoid given that I'd like to keep these changes automated as much as possible.
Is there anything in particular you don't like about the co-authored?
For what it's worth: from our users we hear that they like being able to distinguish between automated and manual changes. The former are broad sweeping changes with a typically narrow scope, whereas large manual changes can be more error-prone. Recording this as co-authored was then one of the few ways to capture this in GitHub.
> Longer term, we might want to look into a flow analysis tool that can tell us when the null checks are unneeded, but baby steps for now.
Agree with the approach at making gradual changes here; We have some capabilities and recipes in this space, that we can explore once we get this initial batch through. I'll likely do a write up on the mailinglist at some point with what's already been done, and what we can possibly do next, if there's interest in further changes.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-pmd-plugin] elharo commented on pull request #124: [MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #124:
URL: https://github.com/apache/maven-pmd-plugin/pull/124#issuecomment-1515133417
You can assign these reviews to me. I might not get to all of them this quickly but they're not hard to review.
I would like it if you can amend the commit messages so they no longer say "Co-authored-by: Moderne".
Longer term, we might want to look into a flow analysis tool that can tell us when the null checks are unneeded, but baby steps for now.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org