Rename Blockly.<language>.variableDB_ to .nameDB_ and add .nameDB_.getUserNames('VARIABLE') and .nameDB_.getUserNames('PROCEDURE') #4845
Conversation
There is significant confusion in names and comments with regards to variables and procedures. `Blockly.Generator.prototype.variableDB_` is a Blockly.Names database, not a variable map. This rename introduces a getter and setter so deprecated references still work. This commit also fixes some comments which are either outright wrong or misleading regarding variable and procedure names.
Also gets rid of hacky name_realm contatination.
Previously isInitialized would remain true after first generation. Also move common init/finish code to parent class.
Same with Object.
Only for JS, Lua, and PHP. Dart and Python need extra logic to split out import statements. Also use ‘this’ instead of fully qualified names in generators.
This enables the generator for any block to see all variable names and procedure names in the whole program, including those that haven’t generated yet.
|
The net result of all of these commits is that in any block's generator one can now call either one of: and get an array of all the user's variable or procedure names. |
core/generator.js
Outdated
| Object.defineProperty(Blockly.Generator.prototype, 'variableDB_', { | ||
| get: function() { | ||
| console.warn('Deprecated use of "variableDB_"; change to "nameDB_"'); | ||
| return this.nameDB_; | ||
| }, | ||
| set: function(x) { | ||
| console.warn('Deprecated use of "variableDB_"; change to "nameDB_"'); | ||
| this.nameDB_ = x; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This seems like a nice way to deprecate something! I'll have to keep this in mind in the future.
Also fix bug in Lua generator where variables were not populated in the nameDB (only language with no variable initialization).
This is potentially dangerous code. And ID could theoretically collide with a name.
There was a problem hiding this comment.
It was helpful that the cleanup changes were in separate commits, but it'd be even easier to review if they were in separate PRs. This also matters because your PR title will be in the release notes, and having variable and procedure names available on demand might be something devs are interested in seeing at a glance from the notes.
core/generator.js
Outdated
| * Getter. | ||
| * @deprecated 'variableDB_' was renamed to 'nameDB_' (May 2021). | ||
| * @this {Blockly.Generator} | ||
| * @return {Blockly.Names} Name database. |
There was a problem hiding this comment.
For new code, we've moved to always providing the nullability modifier (? or !) for non-primitives, to match the current style guide.
There was a problem hiding this comment.
Fixed here, and a few dozen other places.
core/generator.js
Outdated
| * @return {Blockly.Names} Name database. | ||
| */ | ||
| get: function() { | ||
| console.warn('Deprecated use of "variableDB_"; change to "nameDB_"'); |
There was a problem hiding this comment.
We have the Blockly.utils.deprecation.warn function, which would be good to use for consistency and so we can more easily track what we have scheduled for deletion
There was a problem hiding this comment.
Fixed, and changed Blockly.utils.deprecation.warn to also handle properties, not just functions.
Also make Blockly.utils.deprecation.warn apply to properties.
The flip side is that splitting this into multiple PRs would result in one of:
I'm concerned that the current process around release notes is resulting in lower quality code since there's a penalty for resolving side issues on the spot upon discovery. Is there a way we can build informative release notes without discouraging off-topic edits? |
Makes sense. This is one of those times I look longingly over at internal-Google systems and remember how easy it is to have multiple changes going at once. The good ol' days. I probably would have moved all of the cleanup changes to their own PR, and have a separate PR for the non-cleanup changes around name generation to get that balance between clarity/ease of reviewing and workflow, but I realize everyone sets that threshold differently, so it's not a big deal. I do think it's a good idea to update the PR title to mention the generator change though, as it's most relevant from a release notes perspective. I'm not sure how best to improve them, since it needs to be an automatic process, and grabbing every commit title is too granular, imo. We could look into it in our next "non-feature week" though! There's probably some tooling out there for it, or we could have a section in the PR template for release notes and parse that out later. |
NeilFraser
changed the title
BeksOmega
added
the
breaking change
Should be no user-visible changes.