Skip to content

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

Merged
cpcallen merged 4 commits into
RaspberryPiFoundation:developfrom
cpcallen:refactor/7084
Jun 13, 2023
Merged

refactor(generators)!: CodeGenerator per-block-type generator function dictionary#7150
cpcallen merged 4 commits into
RaspberryPiFoundation:developfrom
cpcallen:refactor/7084

Conversation

@cpcallen

@cpcallencpcallen commented Jun 12, 2023

Copy link
Copy Markdown
Collaborator

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.
@cpcallencpcallen added component: generators breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor Refactors code labels Jun 12, 2023
@cpcallen
cpcallen requested a review from a team as a code ownerJune 12, 2023 23:43
@cpcallen
cpcallen requested a review from maribethbJune 12, 2023 23:43
@cpcallen

Copy link
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
CollaboratorAuthor

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.
@cpcallencpcallen changed the title refactor!(generators): CodeGenerator per-block-type generator function dictionaryrefactor(generators)!: CodeGenerator per-block-type generator function dictionaryJun 13, 2023
@github-actionsgithub-actionsBot added breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor Refactors code and removed breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor Refactors code labels Jun 13, 2023
@cpcallen

Copy link
Copy Markdown
CollaboratorAuthor

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

Comment threadcore/generator.ts
@cpcallen
cpcallen merged commit f9c865b into RaspberryPiFoundation:developJun 13, 2023
@cpcallen
cpcallen deleted the refactor/7084 branch June 13, 2023 19:41
@cpcallencpcallen added the deprecation This PR deprecates an API. label Jun 14, 2023
@github-actionsgithub-actionsBot added breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor Refactors code and removed breaking change Used to mark a PR or issue that changes our public APIs. PR: refactor Refactors code 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 RaspberryPiFoundation#7150.
* Fix bad formating in `generators/javascript.js` from PR RaspberryPiFoundation#7153.
* Add missing `@enum` tag that should have been included in PR RaspberryPiFoundation#7160.
* Delete obsolete comment from `generators/python.js` for PR RaspberryPiFoundation#7163.
@cpcallencpcallen 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.
@cpcallencpcallen 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 RaspberryPiFoundation#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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking changeUsed to mark a PR or issue that changes our public APIs.deprecationThis PR deprecates an API.PR: refactorRefactors code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: CodeGenerator per-block-type generator function dictionary

2 participants

@cpcallen@maribethb