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(blocks): Migrate blocks/variables.js and blocks/variables_dynamic.js to TypeScript #7001

Merged
merged 7 commits into from
Apr 24, 2023

Conversation

rachel-fenichel
Copy link
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

Part of #6828

@rachel-fenichel rachel-fenichel requested a review from a team as a code owner April 20, 2023 23:46
@cpcallen cpcallen changed the title refactor: convert variable blocks to typescript refactor(blocks): Migrate blocks/variables.js and blocks/variables_dynamic.js to TypeScript Apr 21, 2023
@cpcallen
Copy link
Contributor

(Retitled for consistency and findability in search.)

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

A few small items. I think most apply to both variables.ts and variables_dynamic.ts, so please check both in case I overlooked it in one or the other.

@@ -249,7 +250,8 @@ const CUSTOM_CONTEXT_MENU_CREATE_VARIABLES_GET_MIXIN = {
* @param options List of menu options to add to.
*/
customContextMenu: function(
this: CustomContextMenuBlock, options: Array<any>) {
this: CustomContextMenuBlock,
options: Array<ContextMenuOption|LegacyContextMenuOption>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure why you're adding this here, since options is not used in the body of the function. Is it needed because this serves as a type declaration that is overridden elsewhere? If so, could the parameter not be omitted here and made optional (for type compatibility) on the override, with a runtime check to ensure it is actually supplied?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

options is used on line 267, where we push the new option onto the array.

blocks/variables.ts Outdated Show resolved Hide resolved
blocks/variables.ts Outdated Show resolved Hide resolved

options.push({
enabled: this.workspace.remainingCapacity() > 0,
text: text,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can be concise here if you like:

Suggested change
text: text,
text,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just inlined type instead since it's only used in this place.

blocks/variables.ts Show resolved Hide resolved
Comment on lines 15 to 25
import {Abstract as AbstractEvent} from '../core/events/events_abstract.js';
import type {ContextMenuOption, LegacyContextMenuOption} from '../core/contextmenu_registry.js';
import * as ContextMenu from '../core/contextmenu.js';
import {FieldVariable} from '../core/field_variable.js';
import * as Extensions from '../core/extensions.js';
import * as Variables from '../core/variables.js';
import type {WorkspaceSvg} from '../core/workspace_svg.js';
import * as xml from '../core/utils/xml.js';
import type {Block} from '../core/block.js';
import {Msg} from '../core/msg.js';
import {createBlockDefinitionsFromJsonArray, defineBlocks} from '../core/common.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

Import ordering:

Suggested change
import {Abstract as AbstractEvent} from '../core/events/events_abstract.js';
import type {ContextMenuOption, LegacyContextMenuOption} from '../core/contextmenu_registry.js';
import * as ContextMenu from '../core/contextmenu.js';
import {FieldVariable} from '../core/field_variable.js';
import * as Extensions from '../core/extensions.js';
import * as Variables from '../core/variables.js';
import type {WorkspaceSvg} from '../core/workspace_svg.js';
import * as xml from '../core/utils/xml.js';
import type {Block} from '../core/block.js';
import {Msg} from '../core/msg.js';
import {createBlockDefinitionsFromJsonArray, defineBlocks} from '../core/common.js';
import * as ContextMenu from '../core/contextmenu.js';
import * as Extensions from '../core/extensions.js';
import * as Variables from '../core/variables.js';
import * as xml from '../core/utils/xml.js';
import {Abstract as AbstractEvent} from '../core/events/events_abstract.js';
import type {Block} from '../core/block.js';
import type {ContextMenuOption, LegacyContextMenuOption} from '../core/contextmenu_registry.js';
import {FieldVariable} from '../core/field_variable.js';
import {Msg} from '../core/msg.js';
import type {WorkspaceSvg} from '../core/workspace_svg.js';
import {createBlockDefinitionsFromJsonArray, defineBlocks} from '../core/common.js';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(But why do the imports from common.js go at the end?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I did it that way because lowercase letters come after uppercase in asciibetical order—but now I realise I'm not actually sure what the rules are (and they no longer exist in the styleguide, since it just says to use the tooling). Maybe we should be ordering by filename? Who knows. Anyway… feel free to do something different if you have a principled reason for doing so.

blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Updated and ready for re-review

blocks/variables.ts Outdated Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
Comment on lines 15 to 25
import {Abstract as AbstractEvent} from '../core/events/events_abstract.js';
import type {ContextMenuOption, LegacyContextMenuOption} from '../core/contextmenu_registry.js';
import * as ContextMenu from '../core/contextmenu.js';
import {FieldVariable} from '../core/field_variable.js';
import * as Extensions from '../core/extensions.js';
import * as Variables from '../core/variables.js';
import type {WorkspaceSvg} from '../core/workspace_svg.js';
import * as xml from '../core/utils/xml.js';
import type {Block} from '../core/block.js';
import {Msg} from '../core/msg.js';
import {createBlockDefinitionsFromJsonArray, defineBlocks} from '../core/common.js';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment on lines 15 to 25
import {Abstract as AbstractEvent} from '../core/events/events_abstract.js';
import type {ContextMenuOption, LegacyContextMenuOption} from '../core/contextmenu_registry.js';
import * as ContextMenu from '../core/contextmenu.js';
import {FieldVariable} from '../core/field_variable.js';
import * as Extensions from '../core/extensions.js';
import * as Variables from '../core/variables.js';
import type {WorkspaceSvg} from '../core/workspace_svg.js';
import * as xml from '../core/utils/xml.js';
import type {Block} from '../core/block.js';
import {Msg} from '../core/msg.js';
import {createBlockDefinitionsFromJsonArray, defineBlocks} from '../core/common.js';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(But why do the imports from common.js go at the end?)

blocks/variables.ts Outdated Show resolved Hide resolved
blocks/variables.ts Show resolved Hide resolved
blocks/variables.ts Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
blocks/variables_dynamic.ts Outdated Show resolved Hide resolved
@rachel-fenichel rachel-fenichel merged commit 96ecfea into google:develop Apr 24, 2023
@rachel-fenichel rachel-fenichel deleted the convert_variables branch April 24, 2023 20:09
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