modelsbuilder / ModelsBuilder.Original

The Community Models Builder for Umbraco
MIT License
114 stars 49 forks source link

Improvement for the API #169

Closed enkelmedia closed 5 years ago

enkelmedia commented 6 years ago

I like the idea of using the strongly types models when working with data using services. This is something that works today:

member.SetValue(Member.GetModelPropertyType(x => x.City).PropertyTypeAlias,"Odense");

This would give me back the alias of the property and I could use it to avoid the "magic strings". But I'm not really happy with the syntax. I would like it to be more like:

member.SetValue(Member.PropertyAliasFor(x=>x.City),"Odense");

Or maybe even simpler

member.SetValue(Member.AliasFor(x=>x.City),"Odense");

I'm not sure if it would make sense to add the "PropertyAliasFor"-method on the generated models but I just wanted to share my idea here. The code would be quite simple.

public static string PropertyAliasFor<TValue>(Expression<Func<Member, TValue>> selector) 
{
   return PublishedContentModelUtility.GetModelPropertyType(GetModelContentType(), selector).PropertyTypeAlias;
}
enkelmedia commented 6 years ago

I came up with something that would work as well if this is not something that we want to put in the core.

This can be used like this:

var propertyAlias = Alias.For<MyDocType>(x=>x.MyPropertyName);

Heres the code if someone would like to use it:

public static class Alias
    {
        public static string For<TModel>(Expression<Func<TModel, object>> selector) where TModel : PublishedContentModel
        {
            // Getting the constant values based on the passed type
            var modelTypeAlias = typeof(TModel).GetField("ModelTypeAlias").GetValue(null).ToString();
            var modelItemType = (PublishedItemType)typeof(TModel).GetField("ModelItemType").GetValue(null);

            var type = PublishedContentType.Get(modelItemType, modelTypeAlias);

            return PublishedContentModelUtility.GetModelPropertyType(type, selector).PropertyTypeAlias;   
        }
    }
PerplexDaniel commented 5 years ago

I agree, it would be great if the syntax gets shorter but I wonder if that way (with reflection etc.) is really necessary. The current way also sometimes actually throws some NullReferenceException during runtime which is really strange. I also think it should actually be in ModelsBuilder core since property aliases are just necessary sometimes (with IContent / IMember / Examine etc).

Since ModelsBuilder already generates source code, I think it would be way faster if ModelsBuilder simply adds the property aliases directly in the source as constant string values. For example, for your Member example, it would look like this:

public partial class Member : PublishedContentModel
{
        // Add aliases in a nested static class (for cleaner syntax)
    public static class PropertyAlias 
    {
        public const string City = "city";
        public const string DateOfBirth = "dateOfBirth";
    }

    // ... Rest of ModelsBuilder generated code
}

// Usage example:
member.SetValue(Member.PropertyAlias.City, "Odense");

Using constants should be a ton more efficient than using reflection together with that System.Linq.Expressions.Expression parameter. Thoughts?

Only issue I can see is if a model contains a property named "PropertyAlias", but I'm sure we can think of something about for that :-)

@zpqrtbnk Adding this is technically feasible right? Considering the aliases and property names are already used during code-gen to create all methods anyway.

zpqrtbnk commented 5 years ago

Technically feasible, and not too complex - there is another task somewhere (need to look for it) - where we plan to generate way more metadata such as, indeed, aliases etc.

zpqrtbnk commented 5 years ago

That other task is #215 - closing this one.

One would do member.SetValue(MB.PropertyAlias.Member.City, "Odense");