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

[1.20.2] Compress the entity library and rename the entity module to entity_extensions #337

Closed
wants to merge 3 commits into from

Conversation

TheGlitch76
Copy link
Member

The entity library had way too many 1 or 2 class modules. This compresses most of those modules into the entity_extensions library (renamed from entity to match block_extensions et al).

I feel that this structure more closely follows the size and scope of modules in our other libraries (and the kind of size we want overall), but I'm absolutely open to feedback on reverting parts or organizing it differently.

The entity library had way too many 1 or 2 class modules. This compresses most of those modules into the `entity_extensions` library (renamed from `entity` to match `block_extensions` et al)
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

This might be the ideal route for many of these Fabric Object Builder API-derived APIs; few things though:

  • it might be better if Quilt Entity Networking API remained split from the rest

@EnnuiL EnnuiL added library: entity Related to the entity library. t: refactor This proposes a refactor. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. labels Aug 25, 2023
@TheGlitch76
Copy link
Member Author

Ennui, I disagree--QuiltExtendedSpawnDataEntity is the definition of an entity extension, and the code footprint is pretty small.

@EnnuiL
Copy link
Contributor

EnnuiL commented Aug 26, 2023

Ennui, I disagree--QuiltExtendedSpawnDataEntity is the definition of an entity extension, and the code footprint is pretty small.

hm, fair enough

Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

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

With Ennui's suggestions its good. Smart idea packing up the modules. Should help with build times everywhere. That is something we should think about more often.

Co-authored-by: Ennui Langeweile <[email protected]>
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

after this last pass? i believe this is ready for merge;
checkstyle violations can be fixed post-merge

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, something that I'm wondering; Why replace the AW with the mixin accessor? i'm just curious about the reasoning

Copy link
Member Author

Choose a reason for hiding this comment

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

IDE import times

@OroArmor OroArmor mentioned this pull request Sep 17, 2023
Co-authored-by: Ennui Langeweile <[email protected]>
@OroArmor
Copy link
Member

This was merged in manually

@OroArmor OroArmor closed this Mar 29, 2024
@TheGlitch76 TheGlitch76 deleted the entity-refactor branch May 29, 2024 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library: entity Related to the entity library. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. t: refactor This proposes a refactor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants