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 locally already.

The details

Resolves

Part of #5026

@gonfunko gonfunko requested a review from a team as a code owner July 16, 2021 21:18
@gonfunko gonfunko requested a review from rachel-fenichel July 16, 2021 21:18
@github-actions github-actions bot added this to the 2021_q3_release milestone Jul 16, 2021
Blockly.Connection.REASON_DIFFERENT_WORKSPACES = 5;
Blockly.Connection.REASON_SHADOW_PARENT = 6;
Blockly.Connection.REASON_DRAG_CHECKS_FAILED = 7;
Connection.CAN_CONNECT = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like an instance of "Exports a class constructor with unrelated static properties", which is one of the triage categories in #5073

Intuitively, I expected that these would be exported directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes: not for this PR, but this looks like it should be an enum. In that form I think it is probably closely-related enough that it could be exported either as a static property of Connection (e.g. Connection.Reason) or beside it (e.g., exports = {Connection, ConnectionReason}). Added to list in #5073.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any action needed here beyond adding to the triage list?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope.

* @private
*/
Blockly.Connection.connectReciprocally_ = function(first, second) {
Connection.connectReciprocally_ = function(first, second) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this one is private and static, I think it can just become a module-internal function named connectReciprocally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for the next few private static functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@gonfunko gonfunko merged commit 627ce99 into RaspberryPiFoundation:goog_module Jul 20, 2021
@gonfunko gonfunko deleted the connection branch July 20, 2021 18:17
@BeksOmega
Copy link
Contributor

As mentioned in the team meeting, this is one of the files I'm going to be modifying. In this case, to deal with shadows.

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.

4 participants