basil commented on code in PR #359:
URL: https://github.com/apache/commons-beanutils/pull/359#discussion_r2255015098
##########
src/main/java/org/apache/commons/beanutils2/converters/AbstractConverter.java:
##########
@@ -66,7 +66,7 @@ public abstract class AbstractConverter<D> implements
Converter<D> {
* @return the given string trimmed and converter to lower-case.
*/
protected static String toLowerCase(final Object value) {
- return StringUtils.toRootLowerCase(toString(value));
+ return toString(value).toLowerCase(Locale.ROOT);
Review Comment:
> Your changes are exactly what I had in mind
Something still doesn't feel right to me, as the new public methods can only
be called on an `AbstractConverter`, not a `Converter`. Maybe I need to move
them to the interface?
> although I didn't notice at all the toLowerCase is a static method, which
would cause a breaking change in the library.
Is that necessarily a problem given that this PR is targetting the `master`
branch (2.0.0-M2), which is in milestone status? The entire library is in a new
package, which is a breaking change from v1. Since the PR that moved to a new
package wasn't rejected, it obviously must be OK to break compatibility in some
cases. Is this one of them? For example, is it OK to make breaking changes,
like renaming the package or this one, until the first 2.0.0 (non-milestone)
version is released?
> Maybe we should leave this method as you originally wrote it, but
deprecate it and introduce a new one?
I could do this if compatibility must be retained across milestone releases.
--
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-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
