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

Added support for expressions in keypath building #3518

Merged
merged 13 commits into from
Mar 6, 2024
Merged

Conversation

papafe
Copy link
Contributor

@papafe papafe commented Feb 9, 2024

Fixes #3494

TODO

  • Changelog entry
  • Tests

Copy link

coveralls-official bot commented Feb 9, 2024

Pull Request Test Coverage Report for Build 8160379640

Details

  • 13 of 14 (92.86%) changed or added relevant lines in 1 file are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+3.0%) to 80.915%

Changes Missing Coverage Covered Lines Changed/Added Lines %
Realm/Realm/DatabaseTypes/KeyPathCollection.cs 13 14 92.86%
Files with Coverage Reduction New Missed Lines %
Realm/Realm/Native/HttpClientTransport.cs 13 50.0%
Totals Coverage Status
Change from base Build 8157624912: 3.0%
Covered Lines: 6676
Relevant Lines: 8107

💛 - Coveralls

Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Looks good - one thing to consider is if we want to do anything about traversing collections. E.g. this expression could enable calls like First, Single, etc.:

MethodCallExpression
{ 
    Arguments:
    [
        MemberExpression
        {
            Expression: not null,
            Member: PropertyInfo argumentPi 
        } innerExpression
    ]
} when typeof(IEnumerable).IsAssignableFrom(argumentPi.PropertyType)
    => GetFullPath(innerExpression)

This would enable stuff like:

// Evaluates to "Friends.FirstName"
KeyPath.For<Person>(p => p.Friends.First().FirstName);

Might be a bit magical though and obviously doesn't include much validation.

Realm/Realm/DatabaseTypes/KeyPathCollection.cs Outdated Show resolved Hide resolved
/// var keyPath = KeyPath.For&lt;Person&gt;(p => p.Dog.Name);
/// </code>
/// </example>
public static KeyPath For<T>(Expression<Func<T, object>> expression)
Copy link
Member

Choose a reason for hiding this comment

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

Is the For name something from Swift/ObjC? I guess I understand where it's coming from, but it's also kind of unusual for a .NET API name.

Copy link
Contributor Author

@papafe papafe Mar 1, 2024

Choose a reason for hiding this comment

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

No, it's something I thought made sense for this case 😁
I also thought it was following a similar "style" of KeyPathCollection.Of. Do you have any other name suggestion? I would much more prefer to have a static method than using the constructor to be honest.

Copy link
Contributor Author

@papafe papafe Mar 5, 2024

Choose a reason for hiding this comment

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

@nirinchev, do you have any suggestion here about an alternative name?

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be impossible to use the ctor here, because we need the generic arg. Perhaps it's a bit verbose, but how about FromExpression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it sounds reasonable :)

Realm/Realm/DatabaseTypes/KeyPathCollection.cs Outdated Show resolved Hide resolved
Realm/Realm/DatabaseTypes/KeyPathCollection.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/NotificationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/NotificationTests.cs Outdated Show resolved Hide resolved
Realm/Realm/DatabaseTypes/KeyPathCollection.cs Outdated Show resolved Hide resolved
Realm/Realm/DatabaseTypes/KeyPathCollection.cs Outdated Show resolved Hide resolved
Co-authored-by: Nikola Irinchev <irinchev@me.com>
@papafe
Copy link
Contributor Author

papafe commented Mar 1, 2024

Looks good - one thing to consider is if we want to do anything about traversing collections. E.g. this expression could enable calls like First, Single, etc.:

MethodCallExpression
{ 
    Arguments:
    [
        MemberExpression
        {
            Expression: not null,
            Member: PropertyInfo argumentPi 
        } innerExpression
    ]
} when typeof(IEnumerable).IsAssignableFrom(argumentPi.PropertyType)
    => GetFullPath(innerExpression)

This would enable stuff like:

// Evaluates to "Friends.FirstName"
KeyPath.For<Person>(p => p.Friends.First().FirstName);

Might be a bit magical though and obviously doesn't include much validation.

Yeah, it would work, but I think it would feel forced in a way, and could give the wrong impression that we're creating an index path for a specific collection index. For now I'd prefer to keep it as it is, and we can come back to it if there's request for more strongly typed keypaths.

…nto fp/kp-expression

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@papafe papafe marked this pull request as ready for review March 1, 2024 12:38
@papafe papafe merged commit 2a67040 into main Mar 6, 2024
67 checks passed
@papafe papafe deleted the fp/kp-expression branch March 6, 2024 10:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate use of expressions for keypaths
2 participants