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

Recursive resolving of generic arguments types in registration of methods in javascript translation #892

Merged
merged 16 commits into from Nov 26, 2020

Conversation

quigamdev
Copy link
Contributor

This PR rewrites resolving of generic arguments in ExpressionHelper.

Example 1:
This registration is already allowed - this is how it was implemented so far.

T Unwrap(T value)
{
     return value.InneValue;
}

Example 2:
This registration is not allowed in the current implementation. This PR makes it possible.

T Unwrap(TransferObject<T> value)
{
     return value.InneValue;
}

@tomasherceg tomasherceg added this to the Version 2.5 milestone Oct 23, 2020
Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comments in code and also please add unit tests for the behavior you have implemented - I don't see a reason to rely on UI tests for this. You can either add them as BindingCompilationTests or JavascriptCompilationTests (first option should be more stable, second also covers the translation to JS).

@quigamdev quigamdev changed the base branch from master to main November 4, 2020 21:38
@quigamdev quigamdev requested a review from exyi November 4, 2020 22:05
@quigamdev quigamdev modified the milestones: Version 2.5, Version 3.0 Nov 4, 2020
@quigamdev quigamdev self-assigned this Nov 4, 2020
@tomasherceg tomasherceg modified the milestones: Version 3.0, Version 3.0 Nice to have Nov 7, 2020
Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, and thanks for the tests. Could you please just have a look at the generic types in arrays?

return method;
var methods = FindValidMethodOveloads(type.GetAllMembers(flags).OfType<MethodInfo>().Where(m => m.Name == name), typeArguments, arguments, namedArgs).ToList();

if (methods.Count == 1) return methods.FirstOrDefault();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: This special case is not needed.

@quigamdev quigamdev requested a review from exyi November 23, 2020 00:01
Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@quigamdev quigamdev merged commit 2b781e0 into main Nov 26, 2020
@quigamdev quigamdev deleted the generic-types-resolving branch November 27, 2020 17:33
@tomasherceg tomasherceg modified the milestones: After 3.0, Version 3.0 Mar 27, 2021
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.

None yet

3 participants