Skip to content

Conversation

@BeksOmega
Copy link
Contributor

@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 alschmiedt and maribethb and removed request for cpcallen July 19, 2021 16:55
{
addCoordinates = false,
addInputBlocks = true,
addNextBlocks = false
Copy link
Contributor 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
Contributor 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.

{
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 RaspberryPiFoundation: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