Conversation
|
Making this draft until we release. |
|
@wnbaum is this more or less aligned with your AST PR? What should I do differently to make your life easier? |
|
I just rebased this with main, ready for review. This are the renames on top of #164 |
@manuq Yes, naming conventions look good and I appreciate you putting the classes in the same I'll give this a more thorough review after I review your data decoupling PR that this is on top of! |
Ah yes, you mentioned it and I then forgot. I will append the block type changes to this PR.
Yes please! This one should be straightforward as it's all renamings. The other one could take more time to review. |
wnbaum
left a comment
There was a problem hiding this comment.
Thanks for renaming the types! After further review I really like the names of everything, and the serialization folder definitely helps organize things better. In my PR I will continue with similar naming conventions like serialized/serilization/definition.
My one request is that I don't think block_definition.gd should be in the main plugin folder, and neither should blocks_catalog.gd. I think we should have a folder addons/block_code/datagen with all of the data generation classes in it. That would include category_factory.gd, blocks_catalog.gd, block_definition.gd. Let me know what you think.
Also you can rebase to main if you want, but it doesn't really matter :)
OK agreed! It would have been better to do that in #164 but that's already merged. So I'll do it here. Previously I had them inside a Also, I would not move
OK done, ready for another review. |
wnbaum
left a comment
There was a problem hiding this comment.
Looking great! One very small change (see comment) and we should be good to go.
Also, if you want you can name the code_generation folder you added data_generation. I think it makes more sense because we are actually generating the data through a script (BlockCatalog). There are some code_generation classes in my PR, but I think we could separate the two if you want.
| undo_redo.add_undo_property(resource.serialized_block, "serialized_props", resource.serialized_block.serialized_props) | ||
| undo_redo.add_do_property(resource.serialized_block, "serialized_props", serialized_props) | ||
| if serialized_props != resource.block_serialized_properties.serialized_props: | ||
| undo_redo.add_undo_property(resource.serialized_block, "serialized_props", resource.block_serialized_properties.serialized_props) |
There was a problem hiding this comment.
This is generating an error because the property name is outdated still I think.
| undo_redo.add_undo_property(resource.serialized_block, "serialized_props", resource.block_serialized_properties.serialized_props) | |
| undo_redo.add_undo_property(resource.block_serialized_properties, "serialized_props", resource.block_serialized_properties.serialized_props) |
There was a problem hiding this comment.
Good catch @wnbaum ! This is the value of code reviews.
And move it to a new serialization/ folder. This resouce will be removed when there are no more properties to serialize, so add a TODO comment.
This resource only had an array property which can be replaced by directly using Array[SerializedBlockTreeNode]
And move it to the serialization/ folder.
And move it to the serialization/ folder Also: remove abandoned function scene_has_bsd_nodes() from block canvas.
And move it to the serialization/ folder.
This will be used in upcoming changes.
For matching the block scene name.
To a new code_generation subfolder.
wnbaum
left a comment
There was a problem hiding this comment.
Looks good to me now! Thanks!
This has the renames for all block serialization resources. Is on top of the UI / definition decouple in #164
https://phabricator.endlessm.com/T35536