Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the internal entity type readers #986

Merged
merged 2 commits into from
Apr 29, 2018

Conversation

Joe4evr
Copy link
Contributor

@Joe4evr Joe4evr commented Mar 19, 2018

After some discussion in #941, I thought that this might be a favorable change.

I decided not to expose the Primitive, Enum, and Nullable readers since there seems to be a bit more complexity involved with those and it seems just too unlikely someone wants to piggyback off of the existing behavior outside of the entities.

@Joe4evr
Copy link
Contributor Author

Joe4evr commented Mar 19, 2018

Open question: Since after this change, it'll be likely that people inherit one of these readers and call the base implementation to see if that succeeds:

var baseResult = await base.ReadAsync(context, input, services);
if (baseResult.IsSuccess)
{
    //.....
}
return baseResult;

would it be prudent to add a new property to TypeReaderResult that returns the best match provided the result was successful?

public object BestMatch => IsSuccess
    ? (Values.Count == 1 ? Values.Single().Value : Values.OrderByDescending(v => v.Score).First().Value)
    : throw new InvalidOperationException("TypeReaderResult was not successful.");

@Nensec
Copy link

Nensec commented Mar 19, 2018

Would't it be better that BestMatch simply returned null rather than throw an exception? Doing the IsSuccess check seems more like something the caller should do, plus it prevents doing a simple ?? fallback. Now it requires an entire try{} catch{}

@Joe4evr
Copy link
Contributor Author

Joe4evr commented Mar 19, 2018

Not really.

  1. The caller should check result.IsSuccess before accessing BestMatch (this is the same principle as when handling arbitrary Streams, where you should check CanRead/CanWrite/CanSeek etc. before performing the respective operation).
  2. There could be the case where null was actually the value given for that parameter and the caller should respect that.

@foxbot foxbot merged commit 660fec0 into discord-net:dev Apr 29, 2018
FiniteReality pushed a commit to FiniteReality/Discord.Net that referenced this pull request May 5, 2018
* Expose the internal entity type readers

* Add BestMatch property to TypeReaderResult for easily accessing the parsed object
@Joe4evr Joe4evr deleted the ExposeTypeReaders branch October 5, 2019 19:43
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.

3 participants