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

Fixed issue with ordinal numbers #19

Merged
merged 5 commits into from
Jan 9, 2024
Merged

Fixed issue with ordinal numbers #19

merged 5 commits into from
Jan 9, 2024

Conversation

hbugdoll
Copy link
Member

@hbugdoll hbugdoll commented Jan 8, 2024

What are you changing/introducing

Ordinal numbers of the form cardinal number followed by point, e.g. "24.", are not detected as floats by StringHelper::isFloat() any longer.
And so they are not casted to integers by StringHelper::typeCastNumeric().

What is the reason for changing/introducing

see issue #18

QA

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues #18

@nadar
Copy link
Contributor

nadar commented Jan 8, 2024

The tests seems to fail:

  1. we need a new tests for your changeset
  2. we have to decide if we can change the unit test, but this is considered as breaking change.

@hbugdoll
Copy link
Member Author

hbugdoll commented Jan 8, 2024

The tests seems to fail:

Sorry, my fault.
I forgot the PHP regex delimiters ^ and $, tests passed now.

  1. we need a new tests for your changeset
  2. we have to decide if we can change the unit test, but this is considered as breaking change.

I've added two new assertions to StringHelperTest::testIsFloat()

  • '.5'true
  • '5.'false

and two new ones to StringHelperTest::testTypeCastNumeric()

  • '.5'0.5
  • '5.''5.'.

Or would it be better to put them into a new separate test StringHelperTest::testOrdinalNumber() regarding backwards compatibility?

@nadar
Copy link
Contributor

nadar commented Jan 9, 2024

Its perfect, thanks. Since you have not changed an existing tests, this looks good to me 👍

@nadar nadar merged commit df002ac into luyadev:master Jan 9, 2024
11 checks passed
@nadar
Copy link
Contributor

nadar commented Jan 9, 2024

this closes #18

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

Successfully merging this pull request may close these issues.

2 participants