-
Notifications
You must be signed in to change notification settings - Fork 635
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
Handle overflow in DesignScript and core nodes #11075
Conversation
Operations that may result in an integer overflow in DesignScript code or core nodes now display a warning when one occurs. The result is maintained as it was, in order to avoid introducing incompatibilities.
Some inconsistencies I found while doing this:
|
@@ -156,6 +156,9 @@ | |||
<data name="FromObjectObsolete" xml:space="preserve"> | |||
<value>This node is obsolete, please use "String from Object"</value> | |||
</data> | |||
<data name="IntegerOverflow" xml:space="preserve"> | |||
<value>The operation resulted in an integer overflow. Its result may be unexpected.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amoursol does this error message sound okay? Any scope for improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term 'overflow' will not make sense to an end user. I suggest we keep it to be technically correct, but change the second sentence:
"The operation resulted in an integer overflow. Integer overflow occurs when a numeric value falls outside of the range in which that integer can be represented - either higher than the maximum, or lower than the minimum. This may cause the integer value to wrap around the maximum or minimum value and generate an unexpected result."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amoursol The error message seem rather long. Would you be ok if I added a linked explanation as an html document for the documentation browser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally cool to have it as a verbose explanation in the Doc Browser, instead yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amoursol Added it to the doc browser, including how to fix and a code sample. Let me know if there is something you would like to change.
Part 2 (after scrolling down a little)
EDIT: I forgot to include the first paragraph before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the help with the docs @Amoursol ! I have just pushed the new version including your changes. I'll post just the end, the previous section did not change:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Did you also include the parenthesis callouts of (Whole number)
for int, and (Decimal place number)
for double?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did. Sorry, I forgot to include a screenshot of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you @mmisol. No need to screenshot 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmisol thanks for fixing this. Aside from the question to Sol, this looks good.
Purpose
Operations that may result in an integer overflow in DesignScript code
or core nodes now display a warning when one occurs. The result is
maintained as it was, in order to avoid introducing incompatibilities.
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@aparajit-pratap @reddyashish
FYIs
@DynamoDS/dynamo