[BUG] Fix crashing on NaN value #382
Merged
Conversation
This file contains 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
Juanpe
reviewed
Apr 12, 2021
| let calculatedNumberOfLines = Int(round(CGFloat(bounds.height - config.paddingInsets.top - config.paddingInsets.bottom) / CGFloat(requiredSpaceForEachLine))) | ||
|
|
||
| let neededLines = round(CGFloat(bounds.height - config.paddingInsets.top - config.paddingInsets.bottom) / CGFloat(requiredSpaceForEachLine)) | ||
| guard !neededLines.isNaN else { return 0 } |
There was a problem hiding this comment.
taking into account the crash description we should check if the value is finite as well. In fact, I think we could use the property isNormal. As we can see in the official doc, this property return false if the value is zero, infinite or NaN
I suggest you modify the guard condition:
Suggested change
| guard !neededLines.isNaN else { return 0 } | |
| guard neededLines.isNormal else { return 0 } |
WDYT?
There was a problem hiding this comment.
Juanpe, I love this, wasn't aware of isNormal, it's a power move for sure and improves the human readability of the code, cheers.
|
Thanks @coreyd303 for your first contribution!! 🎉 I've added a comment, please check it and let me know what do you think 🤙 |
It is possible for the rounded result to be NaN with both the needed lines and the padding values are 0.0, checking for NaN will prevent a crash [part] change isNan to isNormal isNormal provides a more robust check for unexpected values
coreyd303
force-pushed
the
bugfix/crashing-on-nan
branch
from
b692297
to
fdb9bc2
Compare
Juanpe
approved these changes
Apr 12, 2021
|
Great! This PR will be included in the next version 🤙🏼 thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Describe the goal of this PR. Mention any related Issue numbers.
This PR fixes a crash seen when the result of
CGFloat(bounds.height - config.paddingInsets.top - config.paddingInsets.bottom) / CGFloat(requiredSpaceForEachLine)is0Applying
roundto0results inNaNwhich crashes the runtime when converting toInthttps://www.dropbox.com/s/u9id3amj27370bw/Screen%20Shot%202021-04-09%20at%203.04.05%20PM.png?dl=0
Steps to reproduce:
.showAnimatedGradientSkeleton()[or I am assuming any of the .show methods]In our app we dynamically resize a text view depending if there is text within, it might be notable that the text view is embedded in a stackView, but I assume the crash would happen either way if the view size is 0.
Requirements (place an
xin each of the[ ])