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

LANG-1695: NumberUtils::isParseable does not recognise numbers ending in dot #1071

Conversation

orionlibs
Copy link
Contributor

@orionlibs orionlibs commented Jun 29, 2023

LANG-1695 allowed for a number (positive or negative) that has only one decimal point and it is at the end of the input, is treated as a valid number

…al point and it is at the end of the input, is treated as a valid number
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #1071 (78695e1) into master (4756eb0) will decrease coverage by 0.01%.
Report is 99 commits behind head on master.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1071      +/-   ##
============================================
- Coverage     92.13%   92.13%   -0.01%     
  Complexity     7504     7504              
============================================
  Files           195      195              
  Lines         15722    15720       -2     
  Branches       2897     2896       -1     
============================================
- Hits          14486    14483       -3     
  Misses          665      665              
- Partials        571      572       +1     
Files Changed Coverage Δ
...ava/org/apache/commons/lang3/math/NumberUtils.java 94.77% <0.00%> (-0.25%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@garydgregory
Copy link
Member

garydgregory commented Jul 2, 2023

@orionlibs
Thank you for the PR.

Would you please reply in the Jira ticket to the comment https://issues.apache.org/jira/browse/LANG-1695?focusedCommentId=17709278&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17709278

We really need to make sure we are not creating trouble for ourselves ;-)

@orionlibs
Copy link
Contributor Author

orionlibs commented Jul 2, 2023

@garydgregory I am new to open source contribution and a week ago I started with Apache. I applied for a JIRA account, but I was denied one (with no reason given). So, I cannot login to JIRA. Here is the email from apache:
We regret to inform you that, upon reviewing your request for a new Jira account connected with The Apache Software Foundation,
the commons project has chosen to deny the request. We therefore will not create the Jira account.

The following reason was given:
No reason given.

If you wish to appeal this decision, you may do so either directly with the
commons project, or by contacting the ASF Infrastructure team at:
users@infra.apache.org

@orionlibs
Copy link
Contributor Author

@garydgregory also there are many many open tickets in JIRA for many projects that have actually been completed, but they are still open, with no assignees or comments or anything.

@garydgregory
Copy link
Member

@garydgregory I am new to open source contribution and a week ago I started with Apache. I applied for a JIRA account, but I was denied one (with no reason given). So, I cannot login to JIRA. Here is the email from apache: We regret to inform you that, upon reviewing your request for a new Jira account connected with The Apache Software Foundation, the commons project has chosen to deny the request. We therefore will not create the Jira account.

The following reason was given: No reason given.

If you wish to appeal this decision, you may do so either directly with the commons project, or by contacting the ASF Infrastructure team at: users@infra.apache.org

What user name and real name did you use in the application? I'll see if I can find the application and rejection in the archives.

@orionlibs
Copy link
Contributor Author

orionlibs commented Jul 2, 2023

What user name and real name did you use in the application? I'll see if I can find the application and rejection in the archives.

@garydgregory I used this page
https://selfserve.apache.org/jira-account.html
and the info I entered was:
email address = efthymiou.dimitrios1@gmail.com
username = dimitrios.efthymiou
real public name = Dimitrios Efthymiou
tell us the intent for this account = I don't need the account now, but I don't know when I will. It is like when you join a company. They give you access to some basic services. (This was probably the reason, but I didn't know then that I needed access to JIRA in order to update the tickets I worked on. I worked on a few tickets that were unassigned and did not have PR links in them. I know that now. With a JIRA account I can assign myself to the tickets I work on and I can add my PR links in there. I guess that was the reason they rejected me. Maybe I can register once again, but with a better reason this time?)

@garydgregory
Copy link
Member

@garydgregory I am new to open source contribution and a week ago I started with Apache. I applied for a JIRA account, but I was denied one (with no reason given). So, I cannot login to JIRA. Here is the email from apache: We regret to inform you that, upon reviewing your request for a new Jira account connected with The Apache Software Foundation, the commons project has chosen to deny the request. We therefore will not create the Jira account.

The following reason was given: No reason given.

If you wish to appeal this decision, you may do so either directly with the commons project, or by contacting the ASF Infrastructure team at: users@infra.apache.org

@orionlibs
Please apply again for a Jira account, I'll watch for it.

@orionlibs
Copy link
Contributor Author

orionlibs commented Jul 3, 2023 via email

@@ -918,7 +918,8 @@ public void testIsParsable() {
assertFalse(NumberUtils.isParsable("pendro"));
assertFalse(NumberUtils.isParsable("64, 2"));
assertFalse(NumberUtils.isParsable("64.2.2"));
assertFalse(NumberUtils.isParsable("64."));
assertTrue(NumberUtils.isParsable("64."));
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a test that 64.. is false

Copy link
Contributor Author

@orionlibs orionlibs Jul 17, 2023

Choose a reason for hiding this comment

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

@elharo sorry. My understanding of the ticket is that 64. is true i.e. parseable and valid

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. is valid but 64.. (two periods) is not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. I added a test for it

@orionlibs orionlibs changed the title LANG-1695 LANG-1695: NumberUtils::isParseable does not recognise numbers ending in dot Jul 17, 2023
…to LANG-1695-NumberUtils-isParseable-does-not-recognise-values-such-as-2-dot
@@ -1776,7 +1773,7 @@ private static boolean withDecimalsParsing(final String str, final int beginIdx)
if (decimalPoints > 1) {
return false;
}
if (!isDecimalPoint && !Character.isDigit(str.charAt(i))) {
if (!isDecimalPoint && !Character.isDigit(str.charAt(i)) && !(i == str.length() - 1 && str.charAt(i) == '.')) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens in countries that flip the meaning of the comma and period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps there could be another method that takes a Locale object. Otherwise, I don't think the current parser should concern itself with it

Copy link
Contributor

Choose a reason for hiding this comment

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

Per current Javadoc "Parsable numbers include those Strings understood by Integer#parseInt(String), Long#parseLong(String), Float#parseFloat(String)} or Double#parseDouble(String)."

If those methods don't worry about countries that flip the meaning of the comma and period, we shouldn't do it here. That would be a breaking behavior change.

As to another method that uses Locales, that's in java.util so outside the advertised scope for commons-lang.

@@ -1776,7 +1773,7 @@ private static boolean withDecimalsParsing(final String str, final int beginIdx)
if (decimalPoints > 1) {
return false;
}
if (!isDecimalPoint && !Character.isDigit(str.charAt(i))) {
if (!isDecimalPoint && !Character.isDigit(str.charAt(i)) && !(i == str.length() - 1 && str.charAt(i) == '.')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Per current Javadoc "Parsable numbers include those Strings understood by Integer#parseInt(String), Long#parseLong(String), Float#parseFloat(String)} or Double#parseDouble(String)."

If those methods don't worry about countries that flip the meaning of the comma and period, we shouldn't do it here. That would be a breaking behavior change.

As to another method that uses Locales, that's in java.util so outside the advertised scope for commons-lang.

@garydgregory
Copy link
Member

To make things more complicated, in the French locale a non-breaking space can be used to separate some items. I can't recall the details now but it's yet another thing to lookout for.

@elharo
Copy link
Contributor

elharo commented Jul 29, 2023

The more I think about this, the more I'm convinced we should not account for localization. This method is not for the purpose of determining whether a string is plausibly a number. Its purpose is to return true or false depending on whether the string can be parsed by one of the java.lang.Number classes. Those classes don't consider localization, so this one shouldn't either.

@garydgregory
Copy link
Member

@elharo
I agree with you on the purpose of the API.
I'd like to revisit the implementation though: Why are we half-baked-with-bugs parsing the input instead of just try-catching what we claim is input that will or not throw an exception when we a user actually calls on the number APIs?

@elharo
Copy link
Contributor

elharo commented Aug 20, 2023

One purpose of this method is precisely to avoid using exceptions for flow control.

@orionlibs
Copy link
Contributor Author

this PR is still open after half a year

@orionlibs orionlibs closed this by deleting the head repository Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants