Remove unnecessary conditionals and fix falsy property check in imperial conversion#4135
Merged
Merged
Conversation
Falsy checks (e.g. 'if (obj.temperature)') incorrectly skip conversion when the value is 0, which is a valid temperature (0°C = 32°F). Using the 'in' operator checks for property existence instead.
khassel
approved these changes
May 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While removing unnecessary conditionals (for
complimentsreported in https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/27 and forweatherreported in https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/28), I noticed a bug in the imperial conversion path: falsy property checks likeif (imperialWeatherObject.temperature)silently skip the conversion when the value is0. So at exactly 0 °C, the result would show0 °Finstead of the correct32 °F.I wrote unit tests for
convertWeatherObjectToImperialfirst, which confirmed the bug, then fixed it by replacing the falsy checks with theinoperator. Which I also find more intuitive to read.Weather APIs deliver floating point values, so hitting exactly
0.0might be rare in practice - more likely0.1or-0.1, which are truthy and convert fine. That should explain why it went unnoticed by users.