-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 #4830 for getting/setting unnamed fields #4876
Fix #4830 for getting/setting unnamed fields #4876
Conversation
* Returns null when getting unnamed field (value), throws error Field "undefined" not found when attempting to set value of unnamed field
Yes, please add a test to block_test.js, both for this directly and for calling +1 to adding a better error message to Thanks! |
* Added tests for getting/setting field (values) when names are not supplied and test for getting a field value, setting it to a new value, and getting it again. * Added more user-friendly error message for setFieldValue telling the developer that he/she is missing the name rather than Field "undefined" not found.
* Added tests for getting/setting field (values) when names are not supplied and test for getting a field value, setting it to a new value, and getting it again. * Added more user-friendly error message for setFieldValue telling the developer that he/she is missing the name rather than Field "undefined" not found. * Fixed lint error by removing trailing space
Done, there wasn't a test suite for getting/setting field (values) in general so I added one. |
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 submitting this useful improvement that will doubtless help reduce confusion caused by unexpected behaviour!
Although returning null
is better than returning the first unnamed field, it could still result in unintended behaviour elsewhere, so I recommend that getField
(and getFieldValue
and setFieldValue
) throw TypeError
if the caller violates the type specification (that name
must be a string). It also gives us more options in future, should we decide we want a non-string name
argument to do something else.
(It should suffice to have a single type check in getField
.)
* Now throws error for getField/getFieldValue/setFieldValue if provided name is not a string * Changed error to more specific TypeError * Type checking and error message moved up to getField * Tests added/modified to check that non-string types for field names produce type errors
The basics
The details
Resolves
#4830
Proposed Changes
Behavior Before Change
Calling
block.setFieldValue
without supplying the field name for the required second parameter results in the block's first unnamed field being set to the value of the first argument. For example, calling it on a built-intext_print
block replacesprint
with whatever the new value that's passed is. Connected to this, callingblock.getField
orblock.getFieldValue
without supplying a name of the field (whose value) to get returns the block's first unnamed field (value).Behavior After Change
Calling
block.getField
orblock.getFieldValue
without supplying a name of the field (whose value) to get will now result in a return value ofnull
, which is consistent with the documentation that this is what should be produced when the field with that name doesn't exist. Attempting to set a field without providing a name throws an errorField "undefined" not found.
Reason for Changes
These changes put these methods in line with their specification. I.e.,
name
is listed as a required argument forsetFieldValue
andgetField
andgetFieldValue
are supposed to returnnull
when the field with the given name (no name here!) is not found. This also provides the developer with some information that they're probably doing something unintended such as changing the text on the block itself.Test Coverage
Tested locally. Add test to https://github.com/google/blockly/blob/master/tests/mocha/block_test.js?
Documentation
No additional documentation required.
Additional Information
#4830 : In this issue I filed, I mentioned that we should provide a warning or throw an error when getting (the value of) an unnamed field. Should I provide a warning or is returning
null
suffcient? Also, should we throw a perhaps more friendly error when the field name wasn't passed tosetFieldValue
such as missing required argument of name of field to set?