Allow FabricCodecDataProvider to access dynamic registries
#3522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'd argue that since the datagen module only used in dev, it's fine to break the API. |
No, I dont think this is a something we want to do. A few people actually use this in prod as well (for runtime resource gen) |
|
I recommend breaking this in 1.20.5. |
Interesting, I thought people uses different API for that. Still, I don't think the datagen module needs to follow the strict backwards compatibility promise as other modules. |
modmuss50
requested changes
Jan 15, 2024
...pi-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/provider/FabricCodecDataProvider.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/provider/FabricCodecDataProvider.java
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/provider/FabricCodecDataProvider.java
Outdated
Show resolved
Hide resolved
modmuss50
approved these changes
Jan 22, 2024
modmuss50
added
enhancement
modmuss50
reviewed
Jan 28, 2024
...pi-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/provider/FabricCodecDataProvider.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/provider/FabricCodecDataProvider.java
Outdated
Show resolved
Hide resolved
modmuss50
pushed a commit
that referenced
this pull request
Jan 28, 2024
* Add CompletableFuture to FabricCodecDataProvider * Deprecate the old method and constructor, and fix the style * Fix the style for real this time * Add exceptions to the configure methods * Apply suggestions from code review --------- Co-authored-by: modmuss <modmuss50@gmail.com> (cherry picked from commit 5c01334)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR allows
FabricCodecDataProviderto access the dynamic registries if it is passed in the constructor. Now, I have a couple of issues with this implementation myself because I didn't want to cause a breaking change, the biggest one being that if you do not override the correct method in your subclass, it won't generate any files. Using theFabricCodecDataProvider#FabricCodecDataProvider(FabricDataOutput, CompletableFuture, DataOutput.OutputType, String, Codec)}constructor and then theFabricCodecDataProvider#configure(BiConsumer)method won't generate anything as it's never called.This could all be avoided by always requiring a
CompletableFuture<RegistryWrapper.WrapperLookup>in the constructor and by adding saidRegistryWrapper.WrapperLookupto theconfiguremethod, but that would be a breaking change. We would end up with a much more graceful implementation though.