Skip to content
This repository has been archived by the owner on Aug 4, 2024. It is now read-only.

Compare::Number bugfix #30

Closed
wants to merge 1 commit into from

Conversation

desertfox
Copy link

Added Scalar::Util looks_like_number to determine numbers.
Removed incorrect Regex character class operator.
Improved testing to check for string use case.
Added testing for not equals number operator.

Removed incorrect character class operator.
Improved testing to check for string use case.
Added testing for not equals number operator.
@desertfox
Copy link
Author

Should have run a full test sweep, looking over the failures now to see what gives.

@exodist
Copy link
Member

exodist commented Jun 26, 2016

Looks like you made a lot of style changes. I believe the projects perltidy rc is in the project root. Some lines were entirly style changes. I am not super strict about only accepting patches of the main style, but I would prefer not to merge style only line changes that move away from the main style.

@exodist
Copy link
Member

exodist commented Jun 26, 2016

To be clear, looks_like_number is clearly the correct way to go here, so I do want this patch once the kinks are worked out :-)

@desertfox
Copy link
Author

There is no perltidy rc :( I checked, I thought that may have been a issue i just did a vanilla pass on it

@exodist
Copy link
Member

exodist commented Jun 26, 2016

Ok. I will have to fix that. The git repo for Test2 itself (test-simple dist) should have it.

@desertfox desertfox mentioned this pull request Jun 26, 2016
@exodist
Copy link
Member

exodist commented Jun 26, 2016

This PR makes some incorrect assumptions.

unless length($input) && $input =~ m/\S/; This is not a check that the thing provided is a number. This is a check that the thing provided is not an empty or space-only string.

The point of number() in comparisons is to force the system to use numerical comparison instead of string comparison. It is not supposed to enforce that you actually give it numbers. We do not want it to enforce numbers. Sometimes people want to verify that a numerical comparison against $X has a specific result, even if $X is not a number.

This is important for things like object overloading, strings like '0 but true' and all kinds of other crazy things that make "is this a number" an impossible thing to determine after the fact in perl.

number() is used to force the use of numerical comparisons. It is in no way supposed to enforce that the thing is actually a number. That said the name is ambiguous and I can see why the confusion arose. This needs better documentation, and perhaps an is_number() shortcut for the DSL that simply ensures that the value seen is a number.

I am closing this PR as switching these regex's to use looks_like_number leads to code that does not solve the intended problem.

@exodist exodist closed this Jun 26, 2016
@exodist
Copy link
Member

exodist commented Jun 26, 2016

Please don't take my above message as criticisms, and please do continue to submit issues to me. I was also confused as to what my code was doing on a second look, which means it has problems and needs more docs/comments. I would love it if you wanted to submit a PR for those, or for an is_number() check in the DSL.

@exodist
Copy link
Member

exodist commented Jun 26, 2016

also note, I just pushed the perltidyrc to the repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants