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

Fix leading zero handling in toJsonType method #4721

Merged
merged 5 commits into from
Oct 16, 2024
Merged

Fix leading zero handling in toJsonType method #4721

merged 5 commits into from
Oct 16, 2024

Conversation

blackhead1981
Copy link
Contributor

Summary

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

  • Added a check in the toJsonType method to preserve leading zeros in strings.
  • This ensures that strings starting with a leading zero are treated as strings rather than numbers, preventing unwanted data conversion.

- Added a check in the `toJsonType` method to preserve leading zeros in strings.
- This ensures that strings starting with a leading zero are treated as strings rather than numbers, preventing unwanted data conversion.
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @blackhead1981

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@blackhead1981
Copy link
Contributor Author

After a brief check in the debug APK, it seems to be working so far. At least a PIN code with a leading zero works without any issues. I will conduct further tests tomorrow. I kindly ask for your additional testing as well.

@blackhead1981 blackhead1981 marked this pull request as ready for review October 12, 2024 15:36
@blackhead1981
Copy link
Contributor Author

After extensive testing, I was unable to identify any issues. Everything seems to be working as expected without any problems.

@jpelgrom
Copy link
Member

jpelgrom commented Oct 12, 2024

I wonder if it's worth making this more specific? For example 0.5 will now be sent as a string while it would work fine as a number. Thinking of the alarm panel code use case, this change is really only necessary for input starting with 0 that would otherwise be treated as a whole number (and therefore the 0 would be dropped).

    Added a check to identify and parse Double values, allowing for correct handling of decimal numbers (e.g., 0.5).
    Introduced logic to preserve leading zeros in strings that start with "0" and are longer than one character, ensuring these values are treated as strings (e.g., 02 remains as 02).
    Adjusted the parsing order to first check for Double values, followed by handling of Int and Boolean, returning the string if none of these types match.
- Changed the parsing order to first check for integers and then doubles, addressing the issue where integers like "123" were incorrectly interpreted as doubles.
- Updated comments to clarify the behavior of leading zeros and decimal handling.
- Added validation to check if a string starting with '0' followed by a decimal point contains valid digits, returning it as a Double if valid.
- Preserved leading zeros by returning the string representation for inputs like "02".
- Ensured correct parsing behavior for different input formats, returning strings for invalid cases.
@blackhead1981
Copy link
Contributor Author

blackhead1981 commented Oct 13, 2024

@jpelgrom
Thank you for your feedback!

You made a good point about handling inputs like "0.5" and "0." I’ve updated the logic to parse valid decimal numbers starting with '0' as Double, except for '0' itself (which should be an Int), while keeping strings for cases like "02."

As a beginner, I encountered some difficulties, so I appreciate your understanding. I've also verified the logic in the playground: Kotlin Playground Link.

Thanks for your input!

@jpelgrom
Copy link
Member

Your own test highlights an issue:

"02.5",    		// should return as Double
Input: "02.5" => Output: 02.5 (Type: String)

I believe you should be able to fix this by adding \d* to your regex.

(It also says 0.231231 should be returned as a String, which I don't agree with, but it returns a Double as expected.)

@blackhead1981
Copy link
Contributor Author

blackhead1981 commented Oct 15, 2024

I'm actually uncertain whether 02.5 should be treated as a Double or left as a String. My concern is that for cases like passwords or other data starting with 0, treating it as a Double would drop the leading zero, which might have been intentionally written. Shouldn't we assume a leading zero is meant to be preserved when explicitly written, except for cases like 0.***?

As for 0.231231, that's a mistake in my comment. Of course, it should be treated as a Double.

@dshokouhi
Copy link
Member

Shouldn't we assume a leading zero is meant to be preserved when explicitly written

IMO we should preserve and send exactly whatever a user saves. If they want to send "00000.00010" then let them. I think that is the general expectation.

@blackhead1981
Copy link
Contributor Author

Yes, I think so too.
The code should work accordingly then.

Of course, one option could be to ensure it's treated as a string by wrapping the input in quotes. However, I don't think that would be intuitive for most users.

@jpelgrom jpelgrom merged commit 82391cf into home-assistant:master Oct 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Widget Drops Leading Zero (0)
3 participants