search

neoforged / Bus

Event firing and listening framework, based on the event bus concept
GNU Lesser General Public License v2.1
3 stars 8 forks source link

Make abstract events non-listenable #21

Closed Technici4n closed 10 months ago

Technici4n commented 11 months ago

Abstract events cannot be listened to anymore. For use on events like PlayerEvent or the ones that have pre/post subclasses, to reduce user mistakes.

Shadows-of-Fire commented 11 months ago

Should probably just be @Unlistenable. Doesn't make a whole lot of sense to specify "ThisEvent is @UnlistenableEvent", but "ThisEvent is @Unlistenable" reads correctly.

Same thing for https://github.com/neoforged/Bus/pull/20 (should be ICancellable, the Event suffix is superfluous, it can be argued to be more correct there because the idiom is "is-a" rather than "is" but still)

Technici4n commented 11 months ago

UnlistenableEvent and ICancellableEvent should have something in their name to remind users that they belong to the event bus, IMO. There is a very high chance that an annotation like ICancellable would get misused in non-event contexts.

"This event is an @UnlistenableEvent" also reads correctly. ;)

Tslat commented 11 months ago

Seems like double duty for @Internal?

Also I'm with shadows on this - if you do go with it, appending Event to the end doesn't feel right

Technici4n commented 11 months ago

Seems like double duty for @Internal?

Not at all. PlayerEvent is not internal and should not be. This is specifically to prevent listeners being registered to that class, but the class itself is part of the API and can be used by anyone.

Also I'm with shadows on this - if you do go with it, appending Event to the end doesn't feel right

Personal preference I guess

sciwhiz12 commented 11 months ago

I would prefer @Unlistenable as well, to fit in with the naming conventions of other annotations, such as the currently-existing @Cancelable, the JDK's @Serializable, various vendors' @Nullables, and so on.

Additionally, the new annotation should be annotated with @Documented, so it is considered part of the event's public contract and is included in the docs generated by tools such as javadoc.

Technici4n commented 11 months ago

the currently-existing @Cancelable

With #20 it will be ICancellableEvent.

the JDK's @Serializable, various vendors' @Nullables

These annotations are designed to be attachable to any type, unlike the one in this PR. The @UnlistenableEvent annotation is only applicable to event classes, hence the more specific name.

Shadows-of-Fire commented 11 months ago

The @UnlistenableEvent annotation is only applicable to event classes, hence the more specific name.

This is both implied from the package name net.neoforge.bus (albeit less so since it is no longer eventbus) and explicitly stated in the javadoc Annotation that can be added to an event class to prevent listeners from being registered for it. It does not need to be in the annotation name.

Technici4n commented 11 months ago

Might as well be extra clear. It will probably be auto-completed for you by your IDE, so there is no drawback to having a longer name.

Shadows-of-Fire commented 11 months ago

Might as well be extra clear.

No, we don't need to babyproof things.

It will probably be auto-completed for you by your IDE, so there is no drawback to having a longer name.

Keyword probably, eclipse is flaky and I have no idea how VSC handles annotation autocomplete if at all.

Finally, comments on this thread are 3:1 in support of @Unlistenable, and @Unlistenable is more in-line with conventions on annotation naming, and the conventions already used by this project.

Technici4n commented 11 months ago

I just don't see the case for making the annotation shorter, and I don't think there is one. Note that there is @SubscribeEvent (not just @Subscribe).

TelepathicGrunt commented 11 months ago

For shorter names, is there any risk of a user having other annotations that could clash with same name? Does IDE handle that well? Might also make it easier to read and understand when viewing source on GitHub as you can’t easily see javadoc of annotation there in a person’s source

MattiDragon commented 11 months ago

For shorter names, is there any risk of a user having other annotations that could clash with same name? Does IDE handle that well? Might also make it easier to read and understand when viewing source on GitHub as you can’t easily see javadoc of annotation there in a person’s source

In case of conflict it would be just like any other class name conflict: you need to use the full name of one of the classes. IDEs handle it perfectly fine.

Technici4n commented 11 months ago

Switched to plain abstract now.