Fix artificial language matching #253
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #253 +/- ##
==========================================
+ Coverage 82.68% 82.91% +0.23%
==========================================
Files 14 14
Lines 1178 1194 +16
==========================================
+ Hits 974 990 +16
Misses 141 141
Partials 63 63 ☔ View full report in Codecov by Sentry. |
bep
force-pushed
the
art-lang
branch
4 times, most recently
from
0a6b4a9
to
5429ac2
Compare
v2/i18n/bundle.go
Outdated
| base, _ := candidate.Base() | ||
|
|
||
| if base != artTagBase { | ||
| break |
There was a problem hiding this comment.
Are you sure this will actually solve the problem your upstream users are facing? This logic here bails as soon as it sees a non-artificial language, so this workaround will only work if the artificial language is first in the list. Is that always the case with Hugo?
There was a problem hiding this comment.
Hugo creates one Localizer per language, so there will always be only 1 language in that list. But you are right. I added that break as a premature optimization, I will fix it. Thanks.
This works around what seems to be an upstream by implementing a simplified tag matcher for artificual languages. Fixes nicksnyder#252
|
I forced pushed an updated version where I "continue" instead of break. Also added another language to the test. |
|
I would appreciate a merge of this sooner rather than later, as I need to get a fix out for this. |
| return language.NewMatcher(tags) | ||
| } | ||
|
|
||
| return matcher{ |
There was a problem hiding this comment.
Hugo creates one Localizer per language
Oh interesting. Can we limit the scope of this workaround even further? For example, the new matcher should only activate when ALL the tags are artificial?
This is fundamentally an upstream issue so I don’t want to add a lot of non-intuitive complexity to go-i18n itself.
There was a problem hiding this comment.
the new matcher should only activate when ALL the tags are artificial?
No, the tags set here is all the tags in the bundle. There will be a mix of real and artificial languages.
This is fundamentally an upstream issue so I don’t want to add a lot of non-intuitive complexity to go-i18n itself.
I'm just talking for myself here, but to me this is my issue; I had an extremely naive test with only 1 artificial language -- so with that (my) upgrade to the v2 package of go-i18n broke many Hugo sites in the wild, which is why I have said in another thread that I'm releasing a patch for this this week, one way or another.
There was a problem hiding this comment.
OK, I thought this patch was uncontroversial -- I'll try to find another temporary workaround. Thanks.
|
Any hope for progress here? I package Hugo for Fedora, and the use of https://github.com/gohugoio/go-i18n rather than the package here makes that more difficult. We don't have a Fedora -devel package for Hugo's fork, whereas we do have a -devel package for the code here. I am hesitant to package the fork because it bears the warning "This is a short-lived fork to fix a critical upstream issue, do not use." |
This works around what seems to be an upstream by implementing a simplified tag matcher for artificual languages.
Fixes #252
I have run the relevant Hugo benchmarks (which I'm more familiar with) and, not surprisingly, this is faster in the "artificial case", other than that it should be about the same.