Add test-api-v1 (feat: Scoped Events)#5285
Add test-api-v1 (feat: Scoped Events)#5285Kilip1000 wants to merge 59 commits intoFabricMC:26.1.1from
Conversation
# Conflicts: # gradle.properties
# Conflicts: # gradle.properties
Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>
# Conflicts: # fabric-debug-api-v1/src/main/java/net/fabricmc/fabric/mixin/debug/EntityMixin.java # fabric-debug-api-v1/src/main/java/net/fabricmc/fabric/mixin/debug/MobMixin.java
# Conflicts: # gradle.properties
# Conflicts: # gradle.properties
refactor: ServerMobEffectsGameTest now uses EventScopes, an api introduced through the fabric-debug-api-v1 module
There was a problem hiding this comment.
Overall, good for now. Obviously we're going to need docs and some unit tests, but there are some other considerations, most of which are small. One huge concern, though, is that the ServiceLoader system will apply outside of development environments. Is there any way we can ensure it only applies in dev?
I am at least glad that we don't have MODMUSS_RETURN_EVENT and EncapsulationBreaker3000 anymore.
Edit: small nitpick, but could you write a description in compliance with the CONTRIBUTING.md?
...v1/src/main/resources/META-INF/services/net.fabricmc.fabric.impl.base.event.EventFactoryImpl
Outdated
Show resolved
Hide resolved
fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java
Outdated
Show resolved
Hide resolved
fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventPhaseData.java
Show resolved
Hide resolved
fabric-dev-debug-api-v1/src/main/java/net/fabricmc/fabric/api/debug/dev/v1/EventScope.java
Outdated
Show resolved
Hide resolved
fabric-debug-api-v1/src/main/java/net/fabricmc/fabric/api/test/EventScope.java
Outdated
Show resolved
Hide resolved
...rc/testmod/java/net/fabricmc/fabric/test/entity/event/gametest/ServerMobEffectsGameTest.java
Outdated
Show resolved
Hide resolved
...rc/testmod/java/net/fabricmc/fabric/test/entity/event/gametest/ServerMobEffectsGameTest.java
Outdated
Show resolved
Hide resolved
...rc/testmod/java/net/fabricmc/fabric/test/entity/event/gametest/ServerMobEffectsGameTest.java
Show resolved
Hide resolved
...rc/testmod/java/net/fabricmc/fabric/test/entity/event/gametest/ServerMobEffectsGameTest.java
Show resolved
Hide resolved
...rc/testmod/java/net/fabricmc/fabric/test/entity/event/gametest/ServerMobEffectsGameTest.java
Show resolved
Hide resolved
|
Can you move things to a new dev-only module called
|
I'll do this later. |
228df1f to
6d4b306
Compare
sylv256
left a comment
There was a problem hiding this comment.
LGTM, though I'm not sure if we need more events or not. I know Modmuss's original PR had a lot, but I'm pretty sure that was due to its jankiness.
Also, should it be called EventScope#registerScoped, EventScope#registerEvent, or something else entirely?
Earthcomputer
left a comment
There was a problem hiding this comment.
Also not sure about the name of EventScope.registerScoped, but everything else looks fine
I think |
|
I don't like |
|
I think I'll just go with |
modmuss50
left a comment
There was a problem hiding this comment.
Sorry to be blunt but I'm really not keen on this approach, its ripping open the event api in all the ways that go against good API design. I think we can do better where we dont need to undermine the existing event API but its going to need some thought. I dont have the magic solution at the moment in the past I have tried using reflection to achieve this in a way that wraps the event.
Add fabric-dev-debug-api-v1, featuring EventScopes.