T35536 renames

[manuq]

This has the renames for all block serialization resources. Is on top of the UI / definition decouple in #164
https://phabricator.endlessm.com/T35536

[manuq]

Making this draft until we release.

[manuq]

@wnbaum is this more or less aligned with your AST PR? What should I do differently to make your life easier?

[manuq]

I just rebased this with main, ready for review. This are the renames on top of #164

[wnbaum]

@wnbaum is this more or less aligned with your AST PR? What should I do differently to make your life easier?

@manuq Yes, naming conventions look good and I appreciate you putting the classes in the same serialization/ directory as I did. One thing I changed in my PR was renaming Types.BlockType.EXECUTE to Types.BlockType.STATEMENT and added another type for CONTROL. This is because the way I generate the blocks is by instantiating a scene based on the block_type. Cool if you want to do it here with other renames, but its in my PR as well.

I'll give this a more thorough review after I review your data decoupling PR that this is on top of!

[manuq]

@wnbaum is this more or less aligned with your AST PR? What should I do differently to make your life easier?

@manuq Yes, naming conventions look good and I appreciate you putting the classes in the same serialization/ directory as I did. One thing I changed in my PR was renaming Types.BlockType.EXECUTE to Types.BlockType.STATEMENT and added another type for CONTROL. This is because the way I generate the blocks is by instantiating a scene based on the block_type. Cool if you want to do it here with other renames, but its in my PR as well.

Ah yes, you mentioned it and I then forgot. I will append the block type changes to this PR.

I'll give this a more thorough review after I review your data decoupling PR that this is on top of!

Yes please! This one should be straightforward as it's all renamings. The other one could take more time to review.

[wnbaum]

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 :)

[manuq]

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.

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 model subfolder but then the name changed to "definition". I'll use "generation" instead as you request, which is probably what you'll use in your PR. If you don't mind I will name it code_generation instead of codegen to avoid the abbreviation. I find it a good programming practice.

Also, I would not move category_factory.gd yet because it's UI (the functions return arrays of Block scenes). But feel free to do so in your PR when that is not the case anymore.

Also you can rebase to main if you want, but it doesn't really matter :)

OK done, ready for another review.

[wnbaum]

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.

[wnbaum]

This is generating an error because the property name is outdated still I think.

Suggested change

	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)

[manuq]

Good catch @wnbaum ! This is the value of code reviews.

[wnbaum]

Looks good to me now! Thanks!