modelsbuilder / ModelsBuilder.Original

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

Generate code using language fallback #236

Open jvtroyen opened 4 years ago

jvtroyen commented 4 years ago
jvtroyen commented 4 years ago

This is a PR for #235, any way to link them correctly?

ronaldbarendse commented 4 years ago

Isn't this already implemented in https://github.com/OurModelsBuilder/Our.ModelsBuilder/issues/206 and available in the latest source?

jvtroyen commented 4 years ago

Hi @ronaldbarendse , I tried reading up on #206 and related issues/changes. They relate to v4, I worked on v8, can you explain the difference between the v8 and v4 branch? (Just so I work with the correct information) @Shazwazza also commented (on #235, as I messed up the PR)

Back on topic. As a developer, I want to use Model.Title so that I get the title in the current culture, or, if empty, the fallback culture (or chain). The extension-methods discussed mention all kind of possibilities, but fallback would have to be manual, as inModel.Title ?? Model.Title("en-us"), implying knowledge about the model, it's properties and the fallback chain.

The view-designer (who is a separate front-end developer, not the back-end doctype-designer) would have to chain fallback for each property in his views. Alternatively, the backend developer would have to override each of hundreds properties, simply to include fallback. By optionally (driven by a setting or attribute) generating the additional fallback-attribute, all this is avoided and everyone can use Model.Title knowing they will fallback if it's Variant, but they would not have to be aware.

One final example. What if the fallback chain was

If I understand correctly the argumentation for extension methods:

In the view, that means you need to code depending in the given culture, whereas the fallback-parameter should simply be allowed to do it's work and present the use of Model.Title with fallback baked in.

ronaldbarendse commented 4 years ago

@jvtroyen As per https://github.com/OurModelsBuilder/Our.ModelsBuilder/issues/206#issuecomment-526084143, you should get the title within the current culture or using the language fallback (as configured on the languages themselves) with Model.Title(fallback: Fallback.ToLanguage).

If you want a custom language fallback (not using the fallback chain configured on the languages), you should use a default value, e.g. Model.Title(fallback: Fallback.ToDefaultValue, defaultValue: Model.Title("en-US")).

IMHO ModelsBuilder should keep its API as close to the Umbraco core Value extension methods as possible and also try to keep configuration settings to a minimum, so I would recommend closing this PR (even though it has its merits).

jvtroyen commented 4 years ago

I see your point about the usefulness of these extension-methods, but I'd still counter by stating that the people designing the views (not developers, but they manage) would need to use Model.Title(fallback: Fallback.ToLanguage) instead of Model.Title, for EVERY property in EVERY view, instead of relying on the model to do this for you. It's just asking to be forgotten.

On the other hand : I've read many threads and discussions about these extension-methods and version 4. I saw the different branches in the code, but what version on nuget contains these? We're using 8.1 (https://www.nuget.org/packages/Umbraco.ModelsBuilder/). Does it have a new name?

ronaldbarendse commented 4 years ago

The new extension methods in PR #206 aren't released yet, but will probably be included in the next Our.Umbraco.ModelsBuilder release.

You currently don't have to add Fallback.ToLanguage in every view if you implement the property yourself in a partial class with this overload (like your own example in the linked issue #235).

If ModelsBuilder starts generating default fallbacks, it should be generic (and also include Fallback.ToAncestors, possibly Fallback.ToDefaultValue with specified value) and configurable per property (e.g. if you don't want a default fallback for a specific property). This might be implemented using an attribute, similair to RenamePropertyType.

E.g. a DefaultFallbackPropertyType attribute might be added as assembly attribute to change the default fallback of all properties, but also specified/overridden on the MB class for that content/document type or even a specific property alias.

jvtroyen commented 4 years ago

In the meantime, I'm more up to speed with what v4 is.

The things I want to avoid, are:

I like what you're proposing, though. An assembly attribute to pick the default fallback. I'll give implementing it a try, both in v8 and v4.

ronaldbarendse commented 4 years ago

@jvtroyen There's another approach: implement your own IPublishedValueFallback (have a look at the default PublishedValueFallback) and register that in a composer using composition.RegisterUnique<IPublishedValueFallback, CustomPublishedValueFallback>();.

jvtroyen commented 4 years ago

That is an excellent tip. Trying it out right now.

I guess we can close this as we are no longer in the realm of ModelsBuilder.

ronaldbarendse commented 4 years ago

@jvtroyen As per discussion above: this can be implemented by implementing/overriding the property in the generated model or change the registered IPublishedValueFallback. Implementing this the 'right' way would be to use attributes, either assembly, class or property attributes, just like the existing ones: https://github.com/modelsbuilder/ModelsBuilder.Original/wiki/Control-Generation/. So I think this PR can be closed indeed 😉