On the tendency to err by calling no-op methods on Audience

[A248]

It is a very common mistake to accidentally call methods inherited from Audience which are not implemented in any meaningful way on Velocity.

Consider, as evidence, code which appears well and normal to one familiar with Adventure but not closely acquainted with Velocity:

Space_Interprise: player.playSound(Sound.sound(Key.key("minecraft:entity.enderdragon.death"), Sound.Source.MASTER, 1.0f, 1.0f), Sound.Emitter.self()); Any clue why this doesn't work?

While this is a deliberate design decision on the behalf of Adventure to default to "no-op" implementations, it inevitably creates room for error and surprise. Regardless of the motivation behind this decision, it cannot be reversed without violating contract, which is sacrosanct.

Velocity is in a position to mitigate this issue. By leveraging that such mistakes will presumably occur on types from the Velocity API, methods can be overridden to document that, currently, they have no effect. This being done, API users may see the documentation and then realize the mistake.

The problem will remain to some extent, unavoidably; documentation will prevent only some instances. It might be tempting to deprecate these overriding methods; I do not suggest that and would advocate against it.

[astei]

I'm inclined to close this issue, as the entity providing the APIs that are allowed to silently fail is Adventure, not Velocity. There was discussion in the early days of Adventure about allowing clients to query what abilities were supported by an implementation of Audience, and this was explicitly rejected.

I'm keeping this issue open for further discussion but it doesn't seem that Velocity is the right place to discuss this.

[kyngs]

~~I believe that this issue might be solved by having a config option and then overriding all NOOP methods from Audience in ConnectedPlayer and checking if this config option is true. If yes, throw some exception.~~ Nevermind

[kezz]

Throwing exceptions/etc breaks the contract of the adventure API, which specifically notes that unsupported operations fail silently.

[electronicboy]

config based throws would just be stupid and sloppy, capabilities in adventure would be kinda nice but would have its own limitations and such which I can understand why they wouldn't wanna go for that; outside of overriding everything in adventure to slap some form of annotation on it, which would have limited coverage of stuff, i.e. would only cover methods on classes which are intended to be explicitly implemented by stuff, i.e. the player classes

[kyngs]

Throwing exceptions/etc breaks the contract of the adventure API, which specifically notes that unsupported operations fail silently.

OOF, I misread the original issue. I thought the original intention was to implement such a thing, now reading it again, I see this line:

By leveraging that such mistakes will presumably occur on types from the Velocity API, methods can be overridden to document that, currently, they have no effect. This being done, API users may see the documentation and then realize the mistake.

I guess this (^) wouldn't solve anything at all.