-
Notifications
You must be signed in to change notification settings - Fork 225
Limit Commons Lang 3 to test scope #359
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi @basil,
Could you elaborate on where exactly this causes issues in your case? The question of how many call sites justify adding a dependency (like |
|
Hello all, I don't think we need to merge this PR as it is stated as a preference and doesn't fix a bug. I also really don't want to not set a precedent to inline calls from dependencies. I thought OSGi and JPMS were supposed to deal with these types of ideas... |
@garydgregory The original code contains a bug due to an unnecessary |
|
@garydgregory Any update on this PR? |
ppkarwasz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @basil,
After reviewing this PR more closely, I’m not sure code reuse is even a strong argument against accepting it:
- One call site can be delegated entirely to
Introspector. - One call site has been effectively dead code since Java 4.
- One call site should probably perform lowercasing with an explicitly specified
Locale.
src/main/java/org/apache/commons/beanutils2/FluentPropertyBeanIntrospector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/beanutils2/converters/AbstractConverter.java
Outdated
Show resolved
Hide resolved
…Introspector.java Co-authored-by: Piotr P. Karwasz <[email protected]>
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-lang3</artifactId> | ||
| <version>3.18.0</version> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1: version 2 will depend on Lang for other runtime calls like MethodUtils.
@garydgregory If you have vetoed this PR, and there is no clear action item that would cause you to withdraw the veto, then should the PR be closed? |
Commons BeanUtils 1 does not depend on Commons Lang 3, but Commons BeanUtils 2 currently does. This poses a challenge for us because, while we are fine with depending on Commons BeanUtils, we prefer to avoid a dependency on Commons Lang. This is due to our architecture, which exposes dependencies to all third-party plugins.
Since Commons BeanUtils 2 is still in milestone status and has not yet been released, there is an opportunity to address this. This pull request modifies a few minor instances in
src/mainto use native Java Platform functionality instead of Commons Lang 3. The use of Commons Lang 3 in tests remains unchanged.To test this change, I ran
mvn clean verify. CC @ppkarwaszmvn; that'smvnon the command line by itself.