Skip to content
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

refactor(generators)!: CodeGenerator per-block-type generator function dictionary #7150

Merged
merged 4 commits into from
Jun 13, 2023

Conversation

cpcallen
Copy link
Contributor

@cpcallen cpcallen commented Jun 12, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #7084

Proposed Changes

Implements per-block-type generator function dictionary, per proposal #7084.

Behaviour Before Change

CodeGenerator.prototype.blockToCode uses generator function at this[block_type].

Behaviour After Change

CodeGenerator.prototype.blockToCode preferentially uses generator function at this.forBlock[block_type], falling back to this[block_type] if the former does not exist (and issuing a deprecation warning if the latter does).

Code in generators/*/* is updated to install generator functions at the new location.

Reason for Changes

See #7084.

Test Coverage

No changes to manual test procedures anticipated.

Documentation

We should update our generator demos/codelabs to use the new dictionary.

Additional Information

DEPRECATION: as of this PR, adding per-block generator function directly to CodeGenerator instances is deprecated; they should instead be added to the generator's .forBlock dictionary—i.e., existing code of the form:

langGenerator['custom_block_type'] = function(block) {/* ... */};

should be updated to:

langGenerator.forBlock['custom_block_type'] = function(block) {/* ... */};

although the former style will continue to be supported at least until until Blockly v11.

BREAKING CHANGE: this PR prioritises the new .forBlock dictionary when doing lookups, and moves the generator functions we provide from their previous location directly on the CodeGenerator instances to the new .forBlock dictionary on each instance.

Most existing custom generator code should continue to work unchanged for the time being, but any code that references the generator functions we provide—e.g. to replace an implementation we provide or to reuse the implementation for a different block type—will need to be updated:

langGenerator['controls_if'] = function(block) {/* new implementation overriding existing one */};
langGenerator['custom_block_type'] = langGenerator['existing_block_type'];

must be updated to:

langGenerator.forBlock['controls_if'] = function(block) {/* new implementation overriding existing one */};
langGenerator.forBlock['custom_block_type'] = langGenerator.forBlock['existing_block_type'];

Add a dictionary of block generator functions, provisionally
called .forBlock.  Look up generator functions there first, but
fall back to looking up on 'this' (with deprecation notice)
for backwards compatibility.

Also tweak error message generation to use template literal.
@cpcallen cpcallen added component: generators breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor labels Jun 12, 2023
@cpcallen cpcallen requested a review from a team as a code owner June 12, 2023 23:43
@cpcallen cpcallen requested a review from maribethb June 12, 2023 23:43
@cpcallen
Copy link
Contributor Author

Investigating test failure, which reproduces locally. I am slightly confused because it is caused by the change to tests/mocha/generator_test.js—a change which should have resolved any related errors.

@cpcallen
Copy link
Contributor Author

Turns out test in generator_test.js wasn't cleaning up after itself, and was causing failures in insertion_marker_test.js—this is exactly the kind of issue that makes this PR breaking even though the backwards compatibility code should ensure that developers' existing generator code will in most cases continue to work fine.

BREAKING CHANGE: this PR moves the generator functions we provide
from their previous location directly on the CodeGenerator instances
to the new .forBlock dictionary on each instance.

The change in the previous commit does not oblige external
developers to do the same for their custom generators, but
they will need to update any code that references the generator
functions we provide in generators/*/* (e.g. to reuse one of
our functions for a different block type).
Have the blockToCodeTest helper function delete the block generator
functions it adds to generator once the test is done.
The use of generators in insertion_marker_test.js was overlooked
in the earlier commit making such updates, and some test here
were failing due to lack of cleanup in
cleanup in the generator_test.js.
@cpcallen cpcallen changed the title refactor!(generators): CodeGenerator per-block-type generator function dictionary refactor(generators)!: CodeGenerator per-block-type generator function dictionary Jun 13, 2023
@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor and removed breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor labels Jun 13, 2023
@cpcallen
Copy link
Contributor Author

Force-push (and title edit) to make commits conventional.

core/generator.ts Show resolved Hide resolved
@cpcallen cpcallen merged commit f9c865b into google:develop Jun 13, 2023
@cpcallen cpcallen deleted the refactor/7084 branch June 13, 2023 19:41
@cpcallen cpcallen added the deprecation This PR deprecates an API. label Jun 14, 2023
@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor and removed breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor deprecation This PR deprecates an API. labels Jun 14, 2023
cpcallen added a commit to cpcallen/blockly that referenced this pull request Jun 14, 2023
Addresses various nits that escaped previous PRs:

* Add TSDoc for `BlockGenerator` in `core/generator.ts` for PR google#7150.
* Fix bad formating in `generators/javascript.js` from PR google#7153.
* Add missing `@enum` tag that should have been included in PR google#7160.
* Delete obsolete comment from `generators/python.js` for PR google#7163.
@cpcallen cpcallen mentioned this pull request Jun 14, 2023
4 tasks
cpcallen added a commit that referenced this pull request Jun 14, 2023
Addresses various nits that escaped previous PRs:

* Add TSDoc for `BlockGenerator` in `core/generator.ts` for PR #7150.
* Fix bad formating in `generators/javascript.js` from PR #7153.
* Add missing `@enum` tag that should have been included in PR #7160.
* Delete obsolete comment from `generators/python.js` for PR #7163.
@cpcallen cpcallen added the deprecation This PR deprecates an API. label Jun 15, 2023
cpcallen added a commit to cpcallen/blockly that referenced this pull request Jun 27, 2023
This is mostly just chore updating to the new import names
(e.g. BlocklyJavaScript -> javascript.javascriptGenerator),
but the change to Code.checkAllGeneratorFunctionsDefined is
a necessary fix due to the breaking change in PR google#7150,
implementing the .forBlock dictionary.
cpcallen added a commit that referenced this pull request Jun 27, 2023
This is mostly just chore updating to the new import names
(e.g. BlocklyJavaScript -> javascript.javascriptGenerator),
but the change to Code.checkAllGeneratorFunctionsDefined is
a necessary fix due to the breaking change in PR #7150,
implementing the .forBlock dictionary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Used to mark a PR or issue that changes our public APIs. component: generators deprecation This PR deprecates an API. PR: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: CodeGenerator per-block-type generator function dictionary
2 participants