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

fix: Rename Generator to CodeGenerator #6585

Merged
merged 2 commits into from Oct 27, 2022
Merged

fix: Rename Generator to CodeGenerator #6585

merged 2 commits into from Oct 27, 2022

Conversation

NeilFraser
Copy link
Member

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

Naming collision between Blockly.Generator and ES6's Generator.

Proposed Changes

Rename our Generator to CodeGenerator (it's a more descriptive name anyway).

Behaviour Before Change

TypeScript would get confused think that our code generators only had .next .return and .throw methods.

Behaviour After Change

TypeScript should be less grumpy.

Reason for Changes

TypeScript is pretending to be Java.

Test Coverage

I left the tests unchanged after the rename to verify that the old name still works. Then I updated the tests to the new name.

Documentation

We currently don't have any documentation about creating a new CodeGenerator. This is a separate issue.

Additional Information

In a separate commit on this PR I stripped out all the trailing whitespace from our codebase.

Stops collisions with ES6's Generator.
The old Blockly.Generator still exists as a name, but is now deprecated.
@NeilFraser NeilFraser requested a review from a team as a code owner October 27, 2022 09:22
@NeilFraser NeilFraser changed the title Fraser generator fix: Rename Generator to CodeGenerator Oct 27, 2022
@github-actions github-actions bot added the PR: fix Fixes a bug label Oct 27, 2022
@maribethb
Copy link
Contributor

Drive-by comment: We could enable a tslint rule that yells at us when we're shadowing variables. There is an option you have to pass to have it yell about global objects.

Additional drive-by comment: We do have a codelab about generators.

@BeksOmega BeksOmega requested review from BeksOmega and removed request for gonfunko October 27, 2022 21:28
@BeksOmega BeksOmega assigned BeksOmega and unassigned gonfunko Oct 27, 2022
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

I would also be in favor of enabling no-shadow before this gets merged =) Hopefully we never run into this kind of problem again 🤞 But imo, no harm in enabling it anyway!

@NeilFraser
Copy link
Member Author

We aren't actually shadowing anything. Generator (ES6) is indeed an object, but not one that exists in the global namespace. One can't execute new Generator(). The word 'Generator' is just a name that the documentation uses.

However, TypeScript has this fantasy that 'Generator' is a global object, and gets all confused when it sees a user-created object of that name. So we're actually seeing two separate issues with TypeScript here: 1) it should be able to keep track of what Blockly's Generator object is, and 2) it shouldn't even need to keep track of anything since there's no conflict.

@NeilFraser NeilFraser merged commit e90aba9 into develop Oct 27, 2022
@NeilFraser NeilFraser deleted the fraser-generator branch October 28, 2022 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants