Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-6829] Replace any StringUtils#isEmpty(String) and #isNotEmpty(String) #124

Conversation

timtebeek
Copy link
Contributor

…ring)

### [Replace any StringUtils#isEmpty(String) and #isNotEmpty(String)](https://public.moderne.io/recipes/org.openrewrite.java.migrate.apache.commons.lang.IsNotEmptyToJdk)




Co-authored-by: Moderne <team@moderne.io>
Copy link
Contributor Author

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor touch up.

timtebeek referenced this pull request in openrewrite/rewrite Apr 19, 2023
@timtebeek
Copy link
Contributor Author

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.

@elharo elharo merged commit 40fe444 into apache:master Apr 19, 2023
@elharo
Copy link
Contributor

elharo commented Apr 19, 2023

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.

@timtebeek timtebeek deleted the refactor/replace-any-string-utils-is-empty-string-and-is-not-empty-string branch April 19, 2023 21:34
@timtebeek
Copy link
Contributor Author

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.

@elharo
Copy link
Contributor

elharo commented Apr 20, 2023

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

@elharo
Copy link
Contributor

elharo commented Apr 20, 2023

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.

@timtebeek
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants