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: add serialization of child blocks #5120

Merged
merged 8 commits into from
Aug 6, 2021

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Jul 19, 2021

The basics

  • I branched from project-cereal
  • My pull request is against project-cereal
  • My code follows the style guide

Link for Diff: BeksOmega/blockly@cereal/field-upgrades...BeksOmega:cereal/connnected-blocks

The details

Resolves

Work on project cereal
Dependent on #5077

Proposed Changes

Adds support for serializing children of a block.

Does not handle deserialization.

Reason for Changes

Saving state is good.

Test Coverage

Added tests for:

  • All of the different types of connections we want to serialize (value, statement, and next)
  • All of the states those connections could be in (shadow, block, or both)
  • And the config we pass to save ie do save or don't save.

@BeksOmega BeksOmega requested a review from a team as a code owner July 19, 2021 16:55
@BeksOmega BeksOmega requested review from cpcallen and removed request for a team July 19, 2021 16:55
@BeksOmega BeksOmega marked this pull request as draft July 19, 2021 16:55
@BeksOmega BeksOmega requested review from maribethb and alschmiedt and removed request for cpcallen July 19, 2021 16:55
{
addCoordinates = false,
addInputBlocks = true,
addNextBlocks = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It may be better to set this to true by default. Not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, what was the original reasoning for defaulting to false here?

My original assumption was that addNextBlocks would be true, but I am not sure I have a great reason for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally it was because I thought there were a lot of internal things in Blockly that used this behavior. But it turns out it's just copy paste, and everything else wants next blocks. I'll switch it over to true by default.

Copy link
Contributor

@alschmiedt alschmiedt left a comment

Choose a reason for hiding this comment

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

Overall lgtm, I just have some questions about types.

core/serialization/blocks.js Outdated Show resolved Hide resolved
core/serialization/blocks.js Outdated Show resolved Hide resolved
core/serialization/blocks.js Outdated Show resolved Hide resolved
{
addCoordinates = false,
addInputBlocks = true,
addNextBlocks = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, what was the original reasoning for defaulting to false here?

My original assumption was that addNextBlocks would be true, but I am not sure I have a great reason for it.

@alschmiedt alschmiedt self-assigned this Jul 30, 2021
@BeksOmega BeksOmega force-pushed the cereal/connnected-blocks branch from 607d0e8 to 703decb Compare August 6, 2021 14:12
@google-cla google-cla bot added the cla: yes Used by Google's CLA checker. label Aug 6, 2021
@BeksOmega BeksOmega marked this pull request as ready for review August 6, 2021 14:25
@BeksOmega BeksOmega changed the title Add serialization of child blocks feat: add serialization of child blocks Aug 6, 2021
@BeksOmega BeksOmega merged commit 3fb09af into google:project-cereal Aug 6, 2021
BeksOmega added a commit to BeksOmega/blockly that referenced this pull request Sep 13, 2021
* Add tests for serializing connected blocks

* Add serialization of child blocks

* Add tests for not serializing children

* Add options for not serializing children

* Fix types

* Change addNextBlocks to default to true

* Cleanup

* Fix types
@BeksOmega BeksOmega deleted the cereal/connnected-blocks branch September 16, 2021 15:30
BeksOmega added a commit that referenced this pull request Sep 17, 2021
* Add tests for serializing connected blocks

* Add serialization of child blocks

* Add tests for not serializing children

* Add options for not serializing children

* Fix types

* Change addNextBlocks to default to true

* Cleanup

* Fix types
alschmiedt pushed a commit to alschmiedt/blockly that referenced this pull request Sep 20, 2021
* Add tests for serializing connected blocks

* Add serialization of child blocks

* Add tests for not serializing children

* Add options for not serializing children

* Fix types

* Change addNextBlocks to default to true

* Cleanup

* Fix types
alschmiedt pushed a commit that referenced this pull request Sep 20, 2021
* Add tests for serializing connected blocks

* Add serialization of child blocks

* Add tests for not serializing children

* Add options for not serializing children

* Fix types

* Change addNextBlocks to default to true

* Cleanup

* Fix types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by Google's CLA checker.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants