inukshuk / citeproc-ruby

A Citation Style Language (CSL) Cite Processor
103 stars 23 forks source link

Fix style immanent locales #34

Closed fh closed 8 years ago

fh commented 8 years ago

CSL style definitions can contain terms for any number of locales. Currently those terms are completely ignored in favor of the terms defined in a given locale.

This lets them take precedence over terms defined in a locales file. The locale's version acts as a fallback.

This MIGHT break a lot of citation styles. We just do not know how many of those thousands of CSL styles do contain their own term definitions that differ from the locale (mostly en I guess) version. This PR would change the wording in all of them!

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.07%) to 94.491% when pulling cb41286f656989c3fb8d3d410d8d202ed2885215 on easybiblabs:t/style-immanent-locales into 9be891d3d8b141052bd08f311155ce9e53b2015d on inukshuk:master.

inukshuk commented 8 years ago

What do you mean by 'break' styles? Aren't those styles defining terms broken without this PR?

In any case, I just checked, it's about 600 styles which define a locale, so I'd say it's pretty important to enable this as soon as possible, but I'll have to look at it some more over the weekend: at the moment this does not cover all render methods and it also needs to address locale overrides (when you render a multilingual bibliography and individual items are rendered in their respective locale).

fh commented 8 years ago

@inukshuk Yeah - pretty much depends on how you look at it. It will "change" the output of the styles significant for those.

inukshuk commented 8 years ago

Cool, just wanted to make sure I wasn't missing anything. Anyway, this looks good -- I'll merge it now, but will have to make some adjustments for the locale overrides and will push to Rubygems as soon as that's done. Thanks!

inukshuk commented 8 years ago

Hi @walski thanks again for working on this -- just a quick question: did you take a look at this part in the engine implementation. Ideally locales ought to be merged there. I think we should keep your tests and figure out why the initial implementation did not work -- did you take a look at that by any chance?

walski commented 8 years ago

@inukshuk ha, I didn't find that part :) That's why my solution is a little hacky.

The reason why the code you mentioned doesn't work might be a little strange behavior of the merge methods (both the inplace and copying version). As far as I can tell you need to merge the "locale locale" into the "style locale", not the other way round. Maybe take a look at this. I find this a little weird but as I wanted to get this done, I decided to just accept it :)

inukshuk commented 8 years ago

Yeah, I just stepped through the code: the problem is that locale's store just adds term nodes, regardless of whether or not that term already exists. Lookup returns the first matching term it finds, so the terms are actually merged but not overridden. I'll fix that in CSL Ruby and then the original implementation should work.. I'll adjust your tests to use the engine.render instead of just the renderer, because this is really a feature of the engine not the renderer. Sorry about the confusion -- I had a feeling this was already implemented somewhere : )

inukshuk commented 8 years ago

Thanks for bearing with me here. Turns out this was already implemented, but there was a bug when matching terms so the translator/short term was never removed from the immanent locale. I fixed this and adjusted your tests to use engine.render and they all pass.

This (and your other fixes) landed in 1.1.2 (you need to make sure the csl gem is at 1.1.4 for the override to work).

inukshuk commented 8 years ago

Oh, cool, this made quite a few of the tests from the old compatibility testsuite pass!

By the way, since you were working on title case you might be interested to look at these two which are still failing.