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

Allow LINQ Expressions for Binding Expressions #1667

Merged
merged 17 commits into from Jul 10, 2018

Conversation

Projects
None yet
2 participants
@jkoritzinsky
Member

jkoritzinsky commented Jun 10, 2018

  • What does the pull request do?
    • Add new API to allow users to create ExpressionObservers with LINQ Expressions instead of strings.
    • Move string-based binding paths up to Avalonia.Markup.
    • Fix attached property resolution to always resolve the correct property.
  • What is the current behavior?
    • ExpressionObservers can only have their binding paths specified as strings.
    • The AvaloniaPropertyAccessorPlugin tries to resolve an attached property name without the XAML context, so it just makes a guess of which property is the correct property.
  • What is the updated/expected behavior with this PR?
    • ExpressionObservers can be created from LINQ Expressions, ExpressionNodes, or strings.
    • Attached property bindings now use the AvaloniaPropertyAccessorNode class instead with an already-resolved AvaloniaProperty that is resolved via a callback (i.e. XAML type lookup) when built from a string.
  • How was the solution implemented (if it's not obvious)?
    • Stream Bindings in LINQ Expressions are pattern based. A method that is either an instance method or extension method without parameters named StreamBinding generates a stream binding node at that point in the expression.

Checklist:

If the pull request fixes issue(s) list them like this:

Fixes #1652.

@jkoritzinsky jkoritzinsky requested a review from AvaloniaUI/core Jun 10, 2018

@jkoritzinsky jkoritzinsky referenced this pull request Jun 10, 2018

Merged

Remove Sprache dependency #1668

1 of 3 tasks complete
@grokys

Hey @jkoritzinsky I really love this! It's something I intended to do from the beginning but never got round to.

Just a few things:

  1. Indexers don't seem to be working. We should really be testing expressions in the unit tests to catch stuff like this
  2. Should we also allow Binding to be created with expressions? Binding provides a few extra features like binding to the DataContext or to other controls that could be really useful along with strong typing. This could be a separate PR though.
/// <param name="description">
/// A description of the expression. If null, <paramref name="expression"/> will be used.
/// A description of the expression. If null, <paramref name="node"/> will be used.

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Member

This statement doesn't seem to be true.

_root = new WeakReference(root);
}
public static ExpressionObserver Create<T, U>(

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Member

Need an XML doc here.

@@ -78,7 +78,7 @@ public void Should_Not_Keep_Source_Alive_MethodBinding()
Func<ExpressionObserver> run = () =>
{
var source = new { Foo = new MethodBound() };
var target = new ExpressionObserver(source, "Foo.A");
var target = ExpressionObserver.Create(source, o => (Action)o.Foo.A);

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Member

I'm getting an exception thrown here:

System.InvalidOperationException: No coercion operator is defined between types 'Avalonia.LeakTests.ExpressionObserverTests+MethodBound' and 'System.Action'.

Not sure why this isn't showing up on CI?

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Jun 13, 2018

Member

Leak Tests aren't running on CI currently. That's fixed as part of my Cake PR.

@@ -21,7 +22,7 @@ public class ExpressionObserverTests_Property
public async Task Should_Get_Simple_Property_Value()
{
var data = new { Foo = "foo" };
var target = new ExpressionObserver(data, "Foo");
var target = ExpressionObserverBuilder.Build(data, "Foo");

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Member

Should all these tests be using ExpressionObserver.Create rather than ExpressionObserverBuilder? We should really be testing the ExpressionObserver itself here, and putting the ExpressionObserverBuilder stuff in the markup tests I think.

namespace Avalonia.Markup.Parsers
{
public class ExpressionObserverBuilder

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Member

It feels like this should probably be internal - is it public just for the ExpressionObserver tests? If we make them use ExpressionObserver.Create then could this be made internal?

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Jun 18, 2018

Member

Currently it's public so XAML layer classes (MemberSelector and TreeDataTemplate) can use it just like they were using ExpressionObserver beforehand.

@@ -20,7 +21,7 @@ public class ExpressionObserverTests_Indexer
public async Task Should_Get_Array_Value()
{
var data = new { Foo = new [] { "foo", "bar" } };
var target = new ExpressionObserver(data, "Foo[1]");
var target = ExpressionObserverBuilder.Build(data, "Foo[1]");

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Member

If I change this to be:

var target = ExpressionObserver.Create(data, x => x.Foo[1]);

I get:

Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: bar
Actual:   {Error: System.MissingMemberException: Could not find CLR property 'Foo' on 'System.String[]'}

We really need to be testing ExpressionObserver with actual expressions here!

@grokys

This comment has been minimized.

Member

grokys commented Jun 13, 2018

Also, does this PR depend on #1668? A lot of the changes seem to be similar.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Jun 13, 2018

#1668 is dependent on some of the changes in this PR. After this gets merged in the changes over there will clean up a lot.

jkoritzinsky added some commits Jun 18, 2018

Make tests in Avalonia.Base.UnitTests use ExpressionObserver.Create. …
…For tests that require using invalid members or are more tedious to test with expression trees, test them in Avalonia.Markup.UnitTests with ExpressionObserverBuilder.
@grokys

This comment has been minimized.

Member

grokys commented Jun 26, 2018

This is going to conflict with #1694, which do you think it makes more sense to get merged first?

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Jun 26, 2018

I'm on vacation for a week, so probably #1694. Alternatively, all that's left to do here is the XML doc comments, so if you want to put those in we could merge this on approval.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Jul 3, 2018

@grokys I've made all the requested changes. Can you take another review pass when possible?

@jkoritzinsky jkoritzinsky assigned grokys and unassigned jkoritzinsky Jul 3, 2018

@grokys

Mainly a few nits here, but I do wonder about my point 2 above: by making ExpressionObserver only handle expressions and pushing string expressions into Binding, we're making ExpressionNode part of the public API when it feels like an implementation detail.

What would be the cons of making ExpressionObserver handle strings as well?

{
class ExpressionTreeParser
{
private readonly bool enableDataValidation;

This comment has been minimized.

@grokys

grokys Jul 7, 2018

Member

This applies to quite a few places, but elsewhere we use _field with an underscore for fields to follow the .net core coding guidelines.

{
NotifyingBase test = new Class1 { Foo = "Test" };
var target = ExpressionObserver.Create(test, o => ((Class1)o).Foo);

This comment has been minimized.

@grokys

grokys Jul 7, 2018

Member

Oh nice! I was just about to check if this works!

namespace Avalonia.Data.Core.Parsers
{
class ExpressionTreeParser

This comment has been minimized.

@grokys

grokys Jul 7, 2018

Member

Does this need to be a separate class? Could this and ExpressionVisitorNodeBuilder be combined into a single class?

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Jul 8, 2018

Member

I could combine it, but I like having the visitor implementation in its own class. Keeps the messy logic of the visitor separated from the simpler logic in ExpressionTreeParser.

/// <param name="description">
/// A description of the expression. If null, <paramref name="expression"/>'s string representation will be used.
/// </param>
public static ExpressionObserver Create<T, U>(

This comment has been minimized.

@grokys

grokys Jul 7, 2018

Member

Usually, methods come after ctors. I understand why you've done it like this (every ctor has an accompanying static creation method) but I'd rather stick to our standard ordering.

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Jul 9, 2018

Member

I've moved them to after the constructors.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Jul 9, 2018

I have a few reasons why I like having the string support outside of ExpressionObserver.

  1. By having the string support in the Markup layer, the Reader class moves up as well (enabling #1668)
  2. If we have string support in the base layer as a constructor (as done currently), then we make string bindings the easiest path of use. I'd like to push advanced users that want to directly use ExpressionObserver to use type-safe binding expressions if possible.
  3. By making ExpressionNode and subclasses part of the API, we allow advanced users who want to customize the binding system (which is already supported on ExpressionObserver in master) to create custom binding nodes if they so wish.
  4. Everything in Avalonia.Data.Core is already an implementation detail namespace. I don't think exposing anything in that namespace publicly is necessarily bad.
@grokys

grokys approved these changes Jul 10, 2018

@grokys

This comment has been minimized.

Member

grokys commented Jul 10, 2018

Ok, yes those sound like good arguments for doing it this way 👍

@jkoritzinsky jkoritzinsky merged commit 3dd5e3a into AvaloniaUI:master Jul 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jkoritzinsky jkoritzinsky deleted the jkoritzinsky:linq-expression-expressionobserver branch Jul 10, 2018

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