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

AttachmentType Registration Enhancements #4109

Open
wants to merge 12 commits into
base: 1.21.1
Choose a base branch
from

Conversation

forgetmenot13579
Copy link

@forgetmenot13579 forgetmenot13579 commented Sep 21, 2024

Register AttachmentTypes using the builder with consumer pattern.

Today, using the builder pattern creates a Builder<Object> by default, making both the persistent method and the initializer method largely useless, which is especially problematic as this is currently the only way to create an attachment with both persistence and a default value.
DataType
While this can be avoided by explicitly providing type parameters to the static AttachmentRegistry#builder method, type parameterizing methods is unintuitive and should be avoided as an API pattern whenever possible.

The solution implemented in this PR is the addition of the AttachmentRegistry#create(Identifier id, Consumer<Builder<A>>) method. This allows users to configure the (better type inferred) builder directly.
Registration

…of the registered attachment type with a builder.

- Migrate existing creation methods to use the new one under the hood for consistency.
- Moves all null checking from AttachmentRegistry to AttachmentRegistryImpl.BuilderImpl (most of them happened there as well already and were thus redundant).
…substituting the mod ID of the registrant via an entrypoint.
@modmuss50
Copy link
Member

The motivation for this change is to avoid holding onto the mod ID in magic static strings as much as possible.

This does not seem like the solution to this problem, and barely changes anything as it doesnt solve it in every other place you create an identifier. Just create a static method that returns an identifier so you can do MyMod.id("path"). This is a none issue that this change doesnt solve.

@forgetmenot13579
Copy link
Author

I disagree that it is a nonissue, but it is true that solving it here specifically does not make much of a difference when the problem persists across the rest of the ecosystem. I have removed that feature and updated the PR summary accordingly.

Copy link
Contributor

@Syst3ms Syst3ms left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks, this is a reasonable change. I think the entrypoint thing was well out of scope, though. This won't be too big of a hassle to merge into attachment sync.

- Deprecate AttachmentRegistry#builder
- Update existing tests to use #create rather than #builder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants