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

chore: Remove some more uses of AnyDuringMigration #6785

Merged
merged 10 commits into from
Feb 3, 2023

Conversation

gonfunko
Copy link
Contributor

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 #5857

Proposed Changes

Removes uses of AnyDuringMigration from workspace_comment_svg.ts, toolbox.ts, field_colour.ts, field_image.ts, and workspace.ts.

@gonfunko gonfunko requested a review from a team as a code owner January 20, 2023 23:00
@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Jan 20, 2023
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.

The main blocker is that this appears to contain a breaking change.

core/workspace_comment_svg.ts Outdated Show resolved Hide resolved
core/workspace_comment_svg.ts Show resolved Hide resolved
core/toolbox/toolbox.ts Outdated Show resolved Hide resolved
core/toolbox/toolbox.ts Show resolved Hide resolved
core/workspace.ts Show resolved Hide resolved
@@ -154,7 +154,7 @@ export class WorkspaceComment {
* This is not valid if the comment is currently being dragged.
* @internal
*/
getXY(): Coordinate {
getRelativeToSurfaceXY(): Coordinate {
Copy link
Contributor

Choose a reason for hiding this comment

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

This rename would appear to be a breaking change to the API. Is that intended? If so, please update commit message and PR title accordingly. If not, please (at most) deprecate getXY rather than removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is @internal, so I was under the impression we can do whatever we want with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to get @rachel-fenichel and/or @maribethb's take on that. My understanding is that there are some cases where we have previously suggested using (previously) @private methods and now treat them as if they had been @public for the purpose of deciding what's a breaking change.

If this method is not one we've ever suggested people use then SGTM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is safe--we rarely recommend devs do anything with workspace comments, and I don't remember us ever suggesting people use this. And we use the same name (getRelativeToSurfaceXY) for the same functionality in block/block_svg.

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.

LGTM but please check about whether the @internal method really is sufficiently internal.

* Type guard that checks whether an IToolboxItem is an ISelectableToolboxItem.
*/
export function isSelectableToolboxItem(toolboxItem: IToolboxItem):
toolboxItem is ISelectableToolboxItem {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i-dont-understand-is-it-modern

(I'll go look at the typescript docs--I know I've skimmed pass this section before.)

@@ -154,7 +154,7 @@ export class WorkspaceComment {
* This is not valid if the comment is currently being dragged.
* @internal
*/
getXY(): Coordinate {
getRelativeToSurfaceXY(): Coordinate {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is safe--we rarely recommend devs do anything with workspace comments, and I don't remember us ever suggesting people use this. And we use the same name (getRelativeToSurfaceXY) for the same functionality in block/block_svg.

@gonfunko gonfunko merged commit 299016e into google:develop Feb 3, 2023
@gonfunko gonfunko deleted the january-anys branch February 3, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants