[go: nahoru, domu]

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

feat: make ICopyable generic and update clipboard APIs #7348

Merged

Conversation

BeksOmega
Copy link
Collaborator
@BeksOmega BeksOmega commented Aug 1, 2023

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

Fixes #7337

Proposed Changes

  • Updates the ICopyable interface to be generic.
  • Updates the clipboard APIs to take in data instead of being stateful.
  • Makes paste public. Everything else remains internal.

Reason for Changes

  • More informative typing.
  • State is the worst.
  • Necessary if we want people to paste things by passing in data.

Test Coverage

Now that the methods aren't stateful anymore, I could test the API boundary between the clipboard and pasters :D

Documentation

N/A

Additional Information

N/A

Deprecations

Blockly.clipboard.copy and Blockly.clipboard.duplicate have both been deprecated. These were marked @internal so if you are conforming to Blockly's API, you should not be accessing them anyway.

Blockly.clipboard.copy can be replaced with calling toCopyData on the element you want to copy.

Blockly.clipboard.duplicate can be replaced by calling Blockly.clipboard.paste(myElement.toCopyData(), myWorkspace).

@BeksOmega BeksOmega requested a review from a team as a code owner August 1, 2023 21:23
@BeksOmega BeksOmega requested review from maribethb and removed request for a team August 1, 2023 21:23
@github-actions github-actions bot added the PR: feature Adds a feature label Aug 1, 2023
@BeksOmega BeksOmega mentioned this pull request Aug 1, 2023
4 tasks
core/blockly.ts Show resolved Hide resolved
core/clipboard.ts Outdated Show resolved Hide resolved
core/clipboard.ts Outdated Show resolved Hide resolved
core/clipboard.ts Show resolved Hide resolved
core/clipboard.ts Outdated Show resolved Hide resolved
core/clipboard.ts Outdated Show resolved Hide resolved
core/clipboard.ts Show resolved Hide resolved
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Aug 2, 2023
@BeksOmega BeksOmega added the deprecation This PR deprecates an API. label Aug 2, 2023
U extends ICopyData,
T extends ICopyable<U> & IHasWorkspace,
>(toDuplicate: T): T | null {
deprecation.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

would rather not add the deprecation warning until you've fixed the internal usages, but since this is going into a feature branch and not directly into the release i won't stop you

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 have a draft PR fixing all internal usages :D #7352

@BeksOmega BeksOmega merged commit 001d9ff into google:operation-copy-that Aug 3, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This PR deprecates an API. PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants