modelsbuilder / ModelsBuilder.Original

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

Provide option for generated code to use Language Fallback #235

Open jvtroyen opened 4 years ago

jvtroyen commented 4 years ago

Currently, if you want to provide language fallback on properties, you need to provide partial classes and implement it for each Variant property.

It would be nice if there was an appSetting to enable generated code to do this automatically.

[ImplementPropertyType("message")] public string Message => this.Value("message", fallback: Fallback.ToLanguage);

(I've already started work on a PR #236)

Shazwazza commented 4 years ago

IIRC there is a plan within MB (and also within Embedded MB) to generate extension methods for each property. This already occurs for things like Children and Children() and it is valid to have an extension method named the same as a property. The reason why this idea was chosen was to make everything consistent and also to provide the ability to get values by culture since currently this requires an ugly this.Value(x => x.Message, "en-AU") type of syntax, instead it would be this.Message("en-AU"). But these ext methods should include other functionality like fallbacks. Then there is no need for appsettings, etc... it's just adding more extension method being generated.

we came up with this concept with @zpqrtbnk a while back but didn't get around to it. I still think this is a good approach and makes everything consistent.

thoughts?

jvtroyen commented 4 years ago

I created a PR, but it generated a new issue (#236). Sorry, I'm new to the whole PR thing and may have missed how to do the correct flow.

My first thought in regards to your feedback: the extension methods are good in itself, as they open up many possibilities, but developers would still have to "do something" for each property, which is exactly what I'm trying to avoid here. Instead of using this.Message, it would require using this.Message(Fallback.ToLanguage), no?

I would like other developers and those who only make views backed by models developed by others to not have to worry about this, nor have to know which properties are Variant and which are not.

If the generated code is Variant-aware, the properties have fallback incorporated and you are back to simply using this.Message.

Shazwazza commented 4 years ago

Ah i see what you are saying. All good, just not sure if that work will overlap with the ext stuff or not, maybe not!

jvtroyen commented 4 years ago

I think both approaches could live side-by-side, as it's only a change to the existing property-getter, not interfering or inhibiting any extension-methods, as you call them.

Thanks for your thoughts.

Is there a way to link my PR to this issue or is it too late now?

Shazwazza commented 4 years ago

@jvtroyen it's basically linked with the mention of it, else you can edit the very first comment in this thread with a link to it too

ronaldbarendse commented 4 years ago

Like I've commented on PR https://github.com/OurModelsBuilder/Our.ModelsBuilder/pull/236#issuecomment-644147898, if this gets implemented, it should be done as an attribute (similair to RenamePropertyType).

ronaldbarendse commented 4 years ago

One thing to note though: Fallback is a struct, so it can't be added as default parameter value. This might make adding the default fallback a little tricky (especially if you want to combine it with other named parameters, without getting unambiguous method declarations). This also means the fallback isn't exposed in the API/IntelliSense, so it needs to be documented correctly on the generated property.