You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by mureinik <gi...@git.apache.org> on 2017/02/19 09:57:49 UTC
[GitHub] commons-lang pull request #238: Validate's String.format without arguments
GitHub user mureinik opened a pull request:
https://github.com/apache/commons-lang/pull/238
Validate's String.format without arguments
While calling `String.format("some string")` without any additional
arguments is not technically wrong, it's redundant, as it just
returns the same string.
Removing these calls and just using the string instead both cleans up
the code and offers a (very slight) performance gain.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mureinik/commons-lang string-format
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-lang/pull/238.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #238
----
commit 183c2522c5efce9f7d2e6f09e6fe872fd6ff0b8f
Author: Allon Mureinik <am...@redhat.com>
Date: 2017-02-18T09:56:09Z
Validate's String.format without arguments
While calling String.format("some string") without any additional
arguments is not technically wrong, it's redundant, as it just
returns the same string.
Removing these calls and just using the string instead both cleans up
the code and offers a (very slight) performance gain.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by mureinik <gi...@git.apache.org>.
Github user mureinik commented on the issue:
https://github.com/apache/commons-lang/pull/238
Now that I think about it, a cleaner solution might be to add `Object...` arguments to these methods' signatures, and pass it on to the `String.format` calls.
This way, we keep the code 100% backwards compatible, regardless of any `%n`s anyone might have used - any existing calls just pass empty varargs, which is perfectly legal.
The downside is slightly less elegant signatures. If the maintainers prefer this approach, I'd be happy to push such a patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/238
[](https://coveralls.io/builds/10223267)
Coverage remained the same at 94.53% when pulling **183c2522c5efce9f7d2e6f09e6fe872fd6ff0b8f on mureinik:string-format** into **954ade4c1ae2adc0aaac3a1dbe800495c519520c on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/238
@mureinik I think removing these calls should be fine. Specially given that other classes in lang use a normal String, and not `String.format` when there are no parameters, e.g. https://github.com/apache/commons-lang/blob/954ade4c1ae2adc0aaac3a1dbe800495c519520c/src/main/java/org/apache/commons/lang3/ThreadUtils.java#L54
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by mureinik <gi...@git.apache.org>.
Github user mureinik commented on the issue:
https://github.com/apache/commons-lang/pull/238
Actually, it's worse - If your string contains any other formatter other than `%n` these methods would throw a `MissingFormatArgumentException`, as you have no way to pass the arguments to the formatted string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on the issue:
https://github.com/apache/commons-lang/pull/238
Yes, currently the user has to escape single `%`s in the error message with `%%`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on the issue:
https://github.com/apache/commons-lang/pull/238
Yes, lets keep the pull requests as is is. Sorry if I caused the impression of being overtly critical of this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/238
Didn't know about this use of `String.format` @PascalSchumacher , thanks :-)
I'm +1 for merging the pull request. Agree it's unlikely, and not sure if its use was intentional.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang issue #238: Validate's String.format without arguments
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on the issue:
https://github.com/apache/commons-lang/pull/238
Thanks for the pull request (and you other pull requests).
This pull request actually slightly changes the behavior of the methods. There is at least one use for `String.format` without parameters: inserting platform specific line separators with `%n`.
As the javadoc does not say that `String.format` is used on the message and I believe is is unlikely that somebody want would want platform specific line separators here I think we can still merge the pull request.
But maybe somebody else disagrees?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #238: Validate's String.format without arguments
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-lang/pull/238
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---