Skip to content

Conversation

@dkeetonx
Copy link
Contributor

@dkeetonx dkeetonx commented Mar 7, 2025

No description provided.

@dkeetonx
Copy link
Contributor Author

dkeetonx commented Mar 7, 2025

Fixes #2 by removing type declaration in NumericValue constructor

@CodeWithKyrian
Copy link
Owner

Hi @dkeetonx ,

Thanks for identifying this issue! While removing the type declaration would work, I have some reservations about this approach. Removing type declarations reduces type safety, which is something I'd prefer to maintain throughout the codebase.

Instead, I'd like to suggest an alternative solution that preserves type safety in the NumericValue class while still handling numeric strings correctly. The issue occurs in the Environment::convertToRuntimeValues() method where we're passing string values directly to the NumericValue constructor.

A better approach would be to convert numeric strings to actual numeric types before passing them to the constructor, so something like new NumericValue(floatval($input))

Would you be willing to update your PR with this approach instead?

@dkeetonx
Copy link
Contributor Author

Hi @dkeetonx ,

Thanks for identifying this issue! While removing the type declaration would work, I have some reservations about this approach. Removing type declarations reduces type safety, which is something I'd prefer to maintain throughout the codebase.

Instead, I'd like to suggest an alternative solution that preserves type safety in the NumericValue class while still handling numeric strings correctly. The issue occurs in the Environment::convertToRuntimeValues() method where we're passing string values directly to the NumericValue constructor.

A better approach would be to convert numeric strings to actual numeric types before passing them to the constructor, so something like new NumericValue(floatval($input))

Would you be willing to update your PR with this approach instead?

I agree. This a good approach. I just tested it and it looks to be working.

@dkeetonx dkeetonx changed the title Fixes #2 by removing type declaration in NumericValue constructor Fixes #2 by Converting Numeric Strings to Float Values Mar 12, 2025
@CodeWithKyrian CodeWithKyrian merged commit 52f529f into CodeWithKyrian:main Jul 20, 2025
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