Fix #4830 for getting/setting unnamed fields

[jschanker]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide

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-in text_print block replaces print with whatever the new value that's passed is. Connected to this, calling block.getField or block.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 or block.getFieldValue without supplying a name of the field (whose value) to get will now result in a return value of null, 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 error Field "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 for setFieldValue and getField and getFieldValue are supposed to return null 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 to setFieldValue such as missing required argument of name of field to set?

[rachel-fenichel]

Yes, please add a test to block_test.js, both for this directly and for calling setFieldValue with a missing required argument.

+1 to adding a better error message to setFieldValue as well.

Thanks!

[jschanker]

Done, there wasn't a test suite for getting/setting field (values) in general so I added one.

[cpcallen]

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.)