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-6825] Replace StringUtils with Commons Lang3 #832

Conversation

timtebeek
Copy link
Contributor

No description provided.

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

gnodet commented Apr 3, 2023

Can we simply get rid of those calls ?
Switching from a util library to another for just a few methods is not really a win imho.

Some calls can easily be switched to jdk ones like split. The BuildTimeEventSpy.name() method which uses substring and rightpad should be better rewritten to use a StringBuilder if possible, so we're left with a couple of isEmpty or isNotEmpty calls...

@timtebeek
Copy link
Contributor Author

HI @gnodet . Yes I certainly strive to get rid of these calls where I can in the near future, and where the Java language level allows us to do so effectively. Such transformations can (mostly) be automated as well, which is the only way to feasibly do this across 89 repositories. But before we can apply such further automations it helps to first standardize on one variant of StringUtils, rather than the mix of three we have currently. Otherwise we would have to duplicate effort in detecting and migrating method calls, with recipes and change sets more difficult to reason about, and reviews that might require thought rather than clear one to one migrations.

It's best to see this as an ongoing effort of modernization, rather than the intended end state. In MNG-6825 I was asked to create small, easy to reason about PRs, at first going to commons-lang3, and then following that up once we've weened ourselves off of plexus-utils & maven-shared-utils as much as possible.

This PR is one of fifty for this set of change recipes to move towards commons-lang3 at first. I agree that for some projects this can result in a small change set that on it's own isn't adding a lot of value at this time. Yet it's also a necessary step before we can apply more advanced automated migrations across the board such as replacing some of the methods that you've suggested with JDK internals.

Given the above, do you then agree to apply these changes, and others like this, as they are right now?

Alternatively it's also possible to incidentally migrate individual projects towards JDK internals manually already. The automations I'm applying will only trigger when an exact match is found. I do not hope or intend to make a lot of such manual changes myself.

@gnodet
Copy link
Contributor

gnodet commented Apr 4, 2023

HI @gnodet . Yes I certainly strive to get rid of these calls where I can in the near future, and where the Java language level allows us to do so effectively. Such transformations can (mostly) be automated as well, which is the only way to feasibly do this across 89 repositories. But before we can apply such further automations it helps to first standardize on one variant of StringUtils, rather than the mix of three we have currently. Otherwise we would have to duplicate effort in detecting and migrating method calls, with recipes and change sets more difficult to reason about, and reviews that might require thought rather than clear one to one migrations.

It's best to see this as an ongoing effort of modernization, rather than the intended end state. In MNG-6825 I was asked to create small, easy to reason about PRs, at first going to commons-lang3, and then following that up once we've weened ourselves off of plexus-utils & maven-shared-utils as much as possible.

This PR is one of fifty for this set of change recipes to move towards commons-lang3 at first. I agree that for some projects this can result in a small change set that on it's own isn't adding a lot of value at this time. Yet it's also a necessary step before we can apply more advanced automated migrations across the board such as replacing some of the methods that you've suggested with JDK internals.

Given the above, do you then agree to apply these changes, and others like this, as they are right now?

Alternatively it's also possible to incidentally migrate individual projects towards JDK internals manually already. The automations I'm applying will only trigger when an exact match is found. I do not hope or intend to make a lot of such manual changes myself.

Ok, that makes sense. I'll either find the time to get rid of the calls or approve this PR in a timely manner.

@gnodet
Copy link
Contributor

gnodet commented Apr 6, 2023

Superseded with #833

@gnodet gnodet closed this Apr 6, 2023
@timtebeek timtebeek deleted the refactor/mng-6825-replace-string-utils-with-commons-lang-3 branch April 6, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants