Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to use key description #360

Open
naltatis opened this issue Feb 3, 2025 · 6 comments
Open

Unable to use key description #360

naltatis opened this issue Feb 3, 2025 · 6 comments

Comments

@naltatis
Copy link

naltatis commented Feb 3, 2025

Hi, version 2.5.1 broke our translation handling. This code now throws an error but worked previously.

[example]
description = "hello"
foo = "bar"
reserved keys [description] mixed with unreserved keys [foo]

We are using the Key description a lot in our translation. For larger context see https://github.com/evcc-io/evcc/blob/master/i18n/en.toml#L49
What makes description a reserved word? Couldn't find anything about this in the docs.

@andig
Copy link

andig commented Feb 3, 2025

This happened in #320. I couldn't figure where they are defined or what makes them reserved.

@nicksnyder
Copy link
Owner

Let me take a look, the check here might be too agressive

@nicksnyder
Copy link
Owner

nicksnyder commented Feb 4, 2025

@naltatis What version were you on previously?

edit: I see from your project you are on 2.4.1

@nicksnyder
Copy link
Owner

nicksnyder commented Feb 4, 2025

@naltatis Ok I did some investigating and there are two points to make here given a translation file like the one you mentioned:

[example]
description = "hello"
foo = "bar"

First point

With the detection logic that has existed since v2.0.0, this translation file represents a single (malformed) translation with id="example", description="hello", a meaningless (ignored) key "foo", and no actual translation for the id (keys for zero, one, two, few, many, or other are missing).

The only behavior that should have changed in #320 is that this situation now explicitly errors instead of silently interpreting the message in a way that you probably didn't expect. I assume you expected this file to define two messages:
example.description = "hello"
example.foo = "bar"

If you try to fetch these messages in any 2.x release, I don't think you won't be able to. This test case fails on all tagged 2.x releases.

func TestIssue360(t *testing.T) {
	bundle := NewBundle(language.English)
	bundle.RegisterUnmarshalFunc("toml", toml.Unmarshal)
	bundle.MustParseMessageFileBytes([]byte(`
[example]
description = "hello"
foo = "bar"
`), "en.toml")

        // panics because "example.foo" doesn't exist in the bundle (same would happen with "example.description")
	_ = NewLocalizer(bundle, "en").MustLocalize(&LocalizeConfig{MessageID: "example.foo"})
}

Have you been able to successfully fetch these translations somehow? If so, I would love a reproduction.

In short, the first point is that I don't see how this ever worked for you, and the only thing that changed in #320 is that it started explicitly returning an error to tell you that it doesn't work so you would know. This was the goal of the change, given others had run into this problem before: #209

Second point

Could/should the logic be changed to be more permissive and allow description keys like this? It might be possible to really only reserve the "id" key and allow all the others. I can consider this.

@nicksnyder
Copy link
Owner

nicksnyder commented Feb 4, 2025

I have been poking around the source of https://github.com/evcc-io/evcc/ (convenient that it is open source) and infer that the vue frontend is directly reading the translation files without going through go-i18n lib, which is a use case that I honestly had never considered before, and it just kind of works for basic translations because the format happens to be the same (but there is nothing that guarantees this). That is except for when you need to deal with pluralization, but it looks like you don't have any pluralization use in your app.

It looks like the only times you are using go-i18n are in the configure command, which has its own translation files, and session.writeHeader which only needs translations with ids sessions.csv.*.

The underlying problem with your setup is you are feeding translations into the go-i18n Bundle that it doesn't even need (and happen to run into this issue). If you separate out the sessions.csv translations into a separate file (and those look to be needed only by the backend and not in frontend/vue), then that would solve your problem.

I'll consider the second point above more.

@andig
Copy link

andig commented Feb 4, 2025

@nicksnyder thank you for taking the time to investigate. I still feel that I'm missing parts of the reasoning:

With the detection logic that has existed since #170, this translation file represents a single (malformed) translation with id="example", description="hello", a meaningless (ignored) key "foo", and no actual translation for the id (keys for zero, one, two, few, many, or other are missing).

Why is that? Isn't this perfectly valid toml?

In terms of errors vs. silent failures errors are of course preferred. i still find it unexpected that certain keys will error. Are those invalid keys defined somewhere? It should be visible to the developer befor hitting these as errors on runtime?

Update ah, it seems there are internal reasons here: https://github.com/nicksnyder/go-i18n/pull/170/files#diff-9630921e6e60ce29116318c2840bb28b7bbea9f1797fcdde8fb6290186c72e13R9. If our example is valid toml (I guess so) it would still be surprising not being able to use it?

Update 2 finally got it. i18n has much more capabilities than we are using, amongst them pluralisation etc. These share keys with the actual user data which creates the conflict we are seing.

Again, much appreciated, we'll look into changing our logic, too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants