Skip to content

Conversation

@gonfunko
Copy link
Contributor

The basics

  • I branched from goog_module
  • My pull request is against goog_module
  • My code follows the style guide
  • My code is presented in the form suggested in the module
    conversion guide
  • I have run npm test.

The details

Resolves

Part of #5026

Proposed Changes

Converts core/registry.js to goog.module with ES6 const/let.

@gonfunko gonfunko requested a review from a team as a code owner July 26, 2021 16:48
@gonfunko gonfunko requested a review from maribethb July 26, 2021 16:48
@github-actions github-actions bot added this to the 2021_q3_release milestone Jul 26, 2021
core/registry.js Outdated
throw Error(
'Invalid type "' + type + '". The type must be a' +
' non-empty string or a Blockly.registry.Type.');
' non-empty string or a Type.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this error message should retain the namespace for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, updated.

*/
this.name_ = name;
};
exports.Type = Type;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it may be one of the cases where Type ought to be a class in its own file and moved out of this one, and we should add it to the list of weird files to follow up on. Because this isn't just a static function on the registry namespace like many of the other files have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note about this to the tracking issue, I agree it seems reasonable to split it out into its own file.

@gonfunko gonfunko merged commit a827df0 into RaspberryPiFoundation:goog_module Jul 29, 2021
@gonfunko gonfunko deleted the registry branch July 29, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants