ExpressionVisitor?

[mausch]

I noticed that NEST has an old ExpressionVisitor from the times when it was part of the sample codes of LINQ.
This would make sense if NEST were compatible with .NET 3.5, but the DLL that comes with the NuGet package is .NET 4.0 .
Is there any other reason to keep that around?
IMHO .NET's current ExpressionVisitor is cleaner and prevents some invalid rewrites.
I'm asking not just to potentially clean this up but because I have some small code using ExpressionVisitor that I might contribute.

[Mpdreamz]

++ The reason we rolled our own because back when we introduced it .NET 4.0 was still fresh and we did in fact support .NET 3.5. We stopped supporting .NET 3.5 for some time now so I'm definitively open to changing it to the one baked in 4.0.

[mausch]

Cool, however be aware that it's not really a drop-in replacement. Not just because of the different method signatures but also because these new validations mean that an unexpected exception could be thrown, so it would need some testing.
AFAICT the only usage of ExpressionVisitor is PropertyNameResolver, which doesn't seem to have any tests at the moment.

[Mpdreamz]

There are several tests here:

https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Tests/Nest.Tests.Unit/Internals/Inferno/PropertyNameResolverTests.cs

https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Tests/Nest.Tests.Unit/Internals/Inferno/PropertyVisitorTests.cs

https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Tests/Nest.Tests.Unit/Internals/Inferno/MapPropertyNamesForTests.cs

And by proxy of all the serialization tests it should be covered, The only method not tested/covered now is:

public Stack<IElasticPropertyAttribute> ResolvePropertyAttributes(Expression expression)

Which is unused and should be removed.

What we support:

p=>p.Property
p=>p.Collection[0]
p=>p.Collection[0].Property
p=>p.Collection.First()
p=>p.Collection.First().Property
p=>p.Dictionary["hardcoded"]
p=>p.Dictionary["hardcoded"].Property
p=>p.Dictionary[variable]
p=>p.Dictionary[variable].Property
p=>p.Property.Suffix("suffix") //property.suffix
p=>p.Dictionary["hardcoded"].Suffix("suffix") //dictionary.hardcoded.suffix
p=>p.Collection.First().Suffix("suffix") //collection.suffix
p=>p.Collection[0].Suffix("suffix") //collection.suffix
p=>p.Collection.Suffix("suffix") //collection.suffix

(any combination to any depth)

We have an open ticket to support passing variables to Suffix() #998

There is also the MemberInfoResolver which is used by the MapPropertiesFor() on ConnectionSettings() with tests here

If its functionally the same we can pull it in develop if any of the publicly exposed members need changing the PR will have to go to the 2.0 branch since we are under very strict semver requirements due to strongnaming and not changing the assemblyversion to prevent bindingredirects.

[mausch]

Thanks, that was very detailed 👍
My usage is the "dual" of #998 : instead of applying multiple suffixes to a single Expression, I'm applying a single suffix to multiple Expressions. Due to the lack of splicing in C# this needs an ExpressionVisitor. It's just a couple of lines but I thought it might be useful for someone else.