krakenjs / spud

A content store parser, reading a java .properties-like format
Other
14 stars 9 forks source link

Support multiple includes and use @include instead of include for namespace #17

Closed aredridel closed 9 years ago

diegone commented 10 years ago

My opinion about these file-level configurations is that it's going to be flexible and easy to implement but will cause maintainability issues in the long run. Specifically:

totherik commented 10 years ago

It's hard to tell just by looking at the filenames what each file is for.

Is that because of this PR or an artifact of using properties files in general? I'm not sure I understand how this design exacerbates a hypothetical naming problem. Is this insinuating that files are the wrong way to organize content?

it'd be impossible for l10n to know what target country/language to apply to each file based on file/directory name conventions. This will lead to over-translation until a fully automated solution that is aware of the new property is created.

Does that assume these files wouldn't be in the appropriate country/lang directories? I was under the assumption the developer would have to specify the correct fallback, including country/lang.

in general, this approach is in contrast with the rest of kraken, which relies heavily on configuration files

I seem to remember we had discussed keeping the metadata close to where it's used, including both file-level and content-level metadata. Is my memory failing me and had we discussed moving metadata out to separate configuration files? I'm also not sure that I agree that it's in direct contrast with anything kraken-related, and forcing one paradigm into another for the sake of consistency is probably not what we want to do. Ideally, whatever solution we choose will work standalone and depend LESS on kraken than it does already, not more.

diegone commented 10 years ago

My understanding is that today there is an informal agreement that DE/en/foo.properties overrides US/en/foo.properties. This means that purely based on the path, we know we don't have to translate US/en/foo.properties into German.

What this PR allows is to break this 1:1 correlation and one could have a DE/en/bar.properties file including US/en/foo.properties. This means that it's not obvious anymore that US/en/foo.properties shouldn't be translated into German anymore. It sure can be discovered, but it needs some pretty sophisticated tool that reads all files and understand all dependencies to figure that out.

A more common scenario we have today and this PR exacerbate is that because templates may be specialized for a country and they are 1:1 mapped to properties files by convention, we could have a US/en/foo-DE.properties that includes US/en/foo.properties and there is no indication whatsoever that foo-DE should be translated only to German and foo to everything except German.

Regarding configuration, my point was that developers have an expectation to go inside /config and find all configs there, with familiar conventions like environment-based exceptions. They already have to deal with an i18n config that specifies supported and fallback locales. This PR places similar metadata/configuration into the data files themselves. I'm not advocating consistency for consistency sake, but if you think the per-file approach is better, then motivate it and document it for posterity. There are certainly benefits in doing so, just like there would be benefits declaring dust helpers in the dust templates themselves rather than in the config.

aredridel commented 10 years ago

Let's not conflate 'fallback' with 'translation source' with 'override'. They're not the same things.

diegone commented 10 years ago

Maybe it'd help if you explained the difference and how each is supported. It'd also help if you explained how 'include' relates to them since the original motivation for it was to support "key-level fallbacks".

aredridel commented 10 years ago

As we'd discussed by email, includes make sense to prevent duplication of content when languages or, more particularly, regions are derived from another. (The case where 'US/en' and 'GB/en' are mostly similar is an easy to explain case)

This isn't useful for across languages, particularly, since very few strings will be literally the same. That, I think, will require a different piece of metadata -- I'd suggest @source, but that's something for another time.

Then, third, there's a sensible fall-back for the error case where something just wasn't translated for whatever reason -- better than displaying snowman or an empty value. That, right now, is still left to the whole-file fallback and unaddressed. One could abuse @include for this, but papering over errors in non-production modes is definitely not to be encouraged.

I don't think putting this in the main application configuration is smart; it's distant from the place it's used -- not even remotely in the same module -- and changing it across environments is meaningless, and even changing it after the content is created is not terribly likely.

diegone commented 10 years ago

Ok, if that's the definition, then I don't see much use of @include. The example of en-GB doesn't seem valid because that's effectively a translation, no different from de-DE. The fact that between en-US and en-GB there are more strings that are identical than, say, between en-US and de-DE, is irrelevant and solved in the translation processes, not in/by the properties files.

I thought this was the answer to the adaptation requirements and the solve to the missing keys in older translated bundles. If it's not it, then it seems that it's introducing more "abuse" problems than it's solving.

aredridel commented 10 years ago

Wait, "missing keys in older translation bundles"?

diegone commented 10 years ago

Meaning, translation is always a few days behind source content. If someone adds a new string and it hasn't been translated yet, do you want to get it in English or a snowman?

It's the same key-level fallback that you described but not because of a mistake but simply because the translated files are based on an older version of the English files.

aredridel commented 10 years ago

Wow. We've been solving different problems -- developers definitely want an @include, and you'd mentioned that at least in passing as a use-case.

It's useful to help decouple the 1:1 mapping of templates and content bundles and the duplication that implies, while still leaving enough information to be able to figure out where things are coming from.

I think it's worthy, then, to leave, but we'll need to address the fallback problem space a bit more.

mstuart commented 10 years ago

Just wanted to throw my two cents... I was really pushing for this back on https://github.com/krakenjs/makara/issues/11. This would be really nice for reducing duplicated content across a project. Just one example... My project now has ~45 different "Continue" content strings across ~45 "pages". An argument could be made that I should extract that Continue string out into a dust partial because it's most likely a button or link. It just seems really lazy when duplicating strings across content files, but I guess you could deal with this laziness by creating more dust partials. But this is just one example, I don't think creating dust partials are the definitive answer.

As for @diegone's point of making it more complicated for an i18n team to figure out what content needs translated, I can see how it could be a problem because it provides an opportunity for a developer to use words that are more likely to be adapted. But the benefits outweigh the slight chances of consequences...

In the common file, I would expect non-adaptive words like "Continue", "Next", "Select", "Save", "PayPal", etc. More than likely, I'd have one common file per locale and whenever I need a common word, I'd just type "common.continue" in my @pre tag and move on with my life. No need to add a continue key/value for every locale and have it translated for each locale (again, duplicating a translator's work).

diegone commented 10 years ago

@aredridel so you're saying @include could be used so solve the content duplication problem when templates get specialized? So in my example above, a developer would have to create an empty US/en/foo-DE.properties with a single @include property pointing to foo.properties?

@mstuart I agree content sharing and breaking the 1:1 template-property restriction would be good. Wouldn't doing that from the template itself be easier to use? For example with an import directive in the dust template itself, or simply by allowing global content id references in the @pre key (e.g. "/shared/common#continue"). Or if content keys are already globally unique, we should be able to make "common.continue" just work without having to do anything.

aredridel commented 10 years ago

@diegone More like a DE/en/foo.properties with @include=../../US/en/foo.properties, but yes, an empty file save for specializations.

When translated it could maintain the equivalent include in another language, or flatten the bundle to just include the strings. In theory.

aredridel commented 10 years ago

@diegone regarding dust and an import directive, well, that only solves things for dust and that's something that people are showing a lot of interest in escaping.