ababik / remute Goto Github PK

I suggest changing the signature of Remute.With from

public TInstance With<TInstance, TValue>(TInstance source, Expression<Func<TInstance, TValue>> expression, TValue value)

to

public TInstance With<TInstance, TValue>(TInstance source, Expression<Func<TInstance, ValueTuple<TValue,TValue>>> expression)

So that instead of:

remute.With(expected, x => x.FirstName, "Foo");

It will be:

remute.With(expected, x => (x.FirstName, "Foo"));

The advantage here is that the value can depend on the instance, that is you can now do:

remute.With(expected, x => (x.FirstName, "Foo "+x.LastName));

This can also be just an additional overload of With though I feel replacing it is better ...
Any thoughts ?

Remute assumes that the type has a single constructor. however we often have types with multiple constructors (usually one was private and only used for serialization).

It would be nice to have a way to specify the specific constructor to use if there are multiple by providing a Func<IEnumerable<ConstructorInfo>, ConstructorInfo> or via some other mechanism.

PS - Thanks for the library, it's been super useful to us

Given the following:

public class Employee
{
	public Guid Id { get; }

	public string FirstName { get; }

	public string LastName { get; }

	public Employee(Guid id, string firstName, string lastName)
	{
		Id = id;
		FirstName = firstName;
		LastName = lastName;
	}
}

Calling With:

var remute = new Remute.Remute();
var expected = new Employee(Guid.NewGuid(), "John", "Doe");
var actual = remute.With(expected, x => x.FirstName, "Foo");

Results in the following exception:

Expression of type 'System.Guid' cannot be used for return type 'System.Object'
at System.Linq.Expressions.Expression.ValidateLambdaArgs(Type delegateType, Expression& body, ReadOnlyCollection`1 parameters)
at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, String name, Boolean tailCall, IEnumerable`1 parameters)
at Remute.Remute.GetParameterResolvers(Type type, ConstructorInfo constructor)
at Remute.Remute.GetActivationContext(Type type)
at Remute.Remute.With[TInstance,TValue](TInstance instance, Expression`1 expression, TValue value)
at UserQuery.Main()
at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.ThreadHelper.ThreadStart()

One lacking feature that prevents our adoption of this nice library is support for inherited properties. Consider these failing tests:

	public class Foo
	{
		public Foo(string a)
		{
			A = a;
		}

		public string A { get; }
	}
	
	public class Bar : Foo
	{
		public Bar(string a, string b) : base(a)
		{
			B = b;
		}

		public string B { get; }
	}

	public class Tests
	{
		[Fact]
		public static void RemuteInheritance()
		{
			var bar = new Bar("1", "2");

			// throws System.InvalidCastException : Unable to cast object of type 'Tests.Foo' to type 'Tests.Bar'
			bar = bar.Remute(b => b.A, "3"); 

			Assert.Equal("3", bar.A);
		}

		[Fact]
		public static void RemuteInheritanceAttemptedWorkaround()
		{
			var bar = new Bar("1", "2");
			var config = new ActivationConfiguration()
				.Configure<Bar>(b => new Bar(b.A, b.B));
			var remute = new Remute(config);

			// throws System.Exception : Invalid property 'A'. Must be a member of 'Tests.Bar'
			bar = bar.Remute(b => b.A, "3", remute); 
			
			Assert.Equal("3", bar.A);
		}
	}

Here I've got this method's calls.
End here is exception message I've got:

It would be cool, if we can see PropertyName in this, so it would take less time to debug and fix

If I don't need any custom configuration then maybe I shouldn't need to do "new Remute();" every time, perhaps there should be a static default config property on Remute so that I can do something like this:

var actual = Remute.Default.With(expected, x => x.FirstName, "Foo");

Just an idea! (Because I was wondering "why do I need to create new instances of Remute if I'm not doing anything interesting or custom with them).

If I have the line

var remute = new Remute();

then I get an error:

'Remute' is a namespace but is used like a type

I added a line "using Remute;" to the top of the file but VS 2017 removed it as unused.

If we need to apply several mutations one by one to an entity and we don't need any intermediate
results, it would be good if we can achieve it by creating only 1 new instance of entity:
Now:

            var tempDevice = _remute.With(deviceRecord, x => x.EndpointArn, endpointArn);
            var updatedDevice = _remute.With(tempDevice, x => x.DeviceName, deviceName);

Want to:

            var updatedDevice = _remute.With(deviceRecord, x => x.EndpointArn,   endpointArn)   
                                    .With(x => x.DeviceName, deviceName);

Or do we still need System.Collections.Immutable for that? Thanks

I think the InstanceExpressionCacheKey class is not really needed

in
https://github.com/ababik/Remute/blob/master/Remute/Remute.cs#L288

you can instead simply do:
var key = processContext.InstanceExpression.ToString();

It will be useful if in the case of multiple constructors, instead of using ActivationConfiguration().Configure to select the constructor you could simply decorate the constructor you want Remute to choose with an attribute ie [RemuteConstructor]

Thanks!

Can I use this as a nuget package in my commercial product? on what terms?

Hope you don't mind all these notes, just things I notice going through the code ...

• as far as I can tell the Parameter field of ParameterResolver is never used and can be removed ?

• value and target and result seems to be used interchangeably for the same thing which I found really confusing .. maybe stick to target ?

• result is passed about with ref which as far as I can see is not needed as it is reset in TryProcessMemberExpression before each iteration

• what is onEmit ? is this for debugging the library ?

• it would be really cool if With could call itself recursively rather than the current while loop ... not sure if this is possible though ..

Two ideas for optimization:

1. There is no need to add-to / find-in the InstanceExpressionCache cache (that is call ResolveInstance) when mutating a non nested member because we already have the instance in source in that case

2. Instead of creating & compiling an activator and a separate array of parameter resolvers (ActivationContext), create and cache a single lambda with an expression block:

(object instance, object val) => new TheType((MemberType)val, ((TheType)instance).LastName)

ie

(object x, object v) => new Employee((String)v, ((Employee)x).LastName)

This means you don't need to loop over the members if you had a cache hit, this also makes it easy to add a desirable feature where you can mutate multiple members with a single ctor call

Hope this helps
(you can see these ideas in practice here)

While playing around with this, I tried updating a property that can't be updated and I would have expected a runtime exception (rather than it silently ignoring the fact that it can't do what I asked).

Example code below.

If you're happy with the current behaviour then feel free to close the issue, I just wanted to question whether it was intended behaviour or not!

using System;

namespace RemuteTest
{
	class Program
	{
		static void Main(string[] args)
		{
			var remute = new Remute.Remute();
			
			var x = new PersonDetails(123, "Dan");

			// I would expect this to fail at runtime because it's not possible to update the NameAlias property
			x = remute.With(x, _ => _.NameAlias, "DAN");
		}
	}

	public sealed class PersonDetails
	{
		public PersonDetails(int id, string name)
		{
			if (string.IsNullOrWhiteSpace(name))
				throw new ArgumentException($"Null/blank {nameof(name)} specified");

			Id = id;
			Name = name;
		}

		public int Id { get; }
		public string Name { get; }

		public string NameAlias { get { return Name; } }
	}
}

Consider:

public class Employee {
  public string FirstName { get; }
  public string LastName { get; }

  public Employee(int firstName, string lastName) {
    FirstName = "John"+ firstName;
    LastName = lastName;
  }
}

var remute = new Remute();
var expected = new Employee(5, "Doe");
var actual = remute.With(expected, x => x.FirstName, "Foo");

Raises unhandled Exception System.InvalidCastException - "Unable to cast object of type 'System.String' to type 'System.Int32'".

Shouldn't the dictionaries in https://github.com/ababik/Remute/blob/master/Remute/Remute.cs#L28-L29

be protected with a lock ?

Thanks!