Skip to content

Conversation

adamgauthier
Copy link
Contributor

When adding a type reader that replaces a default type reader,
CommandService checks that it replaces a type in its default type
readers or entity type readers (IMessage, IChannel, IRole, IUser). For
entity types, it checks that the target type is the same as the entity
type or it implements its type reader. Adding a type reader for a
default type reader doesn't make much sense and the original intent was
likely to check that the target type is or implements the entity type.

Fixes #1485.

When adding a type reader that replaces a default type reader,
CommandService checks that it replaces a type in its default type
readers or entity type readers (IMessage, IChannel, IRole, IUser). For
entity types, it checks that the target type is the same as the entity
type or it implements its type reader. Adding a type reader for a
default type reader doesn't make much sense and the original intent was
likely to check that the target type is or implements the entity type.
@SubZero0
Copy link
Member

SubZero0 commented Apr 19, 2020

While this indeed fixes replacing the default type reader for the exact type you want it to (unlike how it currently is that doesn't change anything), there's two things to notice.

  1. This will add it in _defaultTypeReaders, not _typeReaders, so CommandService.TypeReaders won't show that custom type reader (this isn't the case with the current implementation, even if it doesn't actually replace it because of how they are prioritized, requiring to manually override with OverrideTypeReaderAttribute). A "band aid fix" would be adding it to the other list, even if it will never be accessed from there internally.

  2. It won't work as a "default type reader", so adding it as _commands.AddTypeReader<IChannel>(new CustomChannelTypeReader(), replaceDefault: true);, will only replace when the command is IChannel, not IMessageChannel or any other. This issue is for both how it currently is and after this change.

Tl;dr: it does replace the default type reader for the specific type, but doesn't list the custom type reader nor work as an entity type reader (this being another issue currently too).

EDIT: Seeing this again, it seems indeed a needed fix, but not to "Fix add type reader for entity implementations", since entity type readers added by the user continue not working (understand entity as a reader that works for themselves and classes that implement them).

@adamgauthier
Copy link
Contributor Author

@SubZero0 Hey, thank you for the quick response. I agree with your points and there are definitely changes that are needed to improve the type reader system, some of which might require breaking changes.

This PR doesn't address those and simply aims to bring the addition of type readers for types that implement IMessage, IChannel, IRole, or IUser in line with the addition of type readers for these types directly, which was the intended behavior with the current implementation.

As for the commit subject, sometimes it's hard to summarize the changes concisely, I now see what "entity" means more clearly, I was simply referring to types that implement types that are added as entity type readers by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to overwrite default type reader for type that implements IMessage, IChannel, IRole or IUser
3 participants