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

Add support for comment (ignorable) column in translation csv #84569

Conversation

jsjtxietian
Copy link
Contributor

For godotengine/godot-proposals#8183

We alreay have #83600 merged, so add support for this proposal is trivial, just ignore the name starts with _ and keep warning other invalid names.

@IntangibleMatter
Copy link
Contributor

Why is this marked as 4.3? It's a small QOL added on to a bug fix, no reason it couldn't be put in the next beta of 4.2.

@Calinou
Copy link
Member

Calinou commented Nov 7, 2023

Why is this marked as 4.3? It's a small QOL added on to a bug fix, no reason it couldn't be put in the next beta of 4.2.

4.2 is in feature freeze, so any new features (including QOL improvements) can only be considered for 4.3 at the earliest. We know this policy can feel strict, but it ensures we can focus on stabilizing 4.2 for release and avoid regressions. This is also why the release cycle now features more frequent minor releases, so that there's less of a wait between minor releases.

@IntangibleMatter
Copy link
Contributor

Why is this marked as 4.3? It's a small QOL added on to a bug fix, no reason it couldn't be put in the next beta of 4.2.

4.2 is in feature freeze, so any new features (including QOL improvements) can only be considered for 4.3 at the earliest. We know this policy can feel strict, but it ensures we can focus on stabilizing 4.2 for release and avoid regressions. This is also why the release cycle now features more frequent minor releases, so that there's less of a wait between minor releases.

Ahhh, that makes sense. I always assumed feature freeze referred to features that were a bit more complex, and never considered QOL things that don't really impact editor workflows that much to be "features".

Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know who is supposed to review this. I don't have much experience with translations, but this looks like a useful feature (It needs to be documented!) and nothing wrong with the technical aspects of this code. Since this is a simple enough change and not compat breaking AFAIK (no valid locales start with _), I don't see why it would be a problem to ship.

An aside here, but this clearly identifies a process error: we shouldn't be failing to approve something due to feature freeze and then sleep on the change until the next feature freeze, especially something this simple.

I know there is some discussion internally on improving the processes related to feature freeze, and I'll bring up this PR as a prime candidate of this since it seems to have been forgotten.

Anyway good thing I caught this before feature freeze. I will try and bring it up in the asset pipeline meeting and/or see if I can find out who works on translations and would be a better reviewer.

@jsjtxietian
Copy link
Contributor Author

It needs to be documented

Yes it does! I will make a pr to the doc repo as soon as this pr is merged.

@akien-mga
Copy link
Member

Is there any kind of precedent for this kind of syntax for "ignorable" columns in CSV?
I'm worried about both discoverability, and potential breakage for people who might be using column titles with underscores as a personal preference/convention.

@timothyqiu
Copy link
Member

potential breakage for people who might be using column titles with underscores as a personal preference/convention.

The first row of a CSV translation file should be locale identifiers. Starting with an underscore is already an invalid locale.

@IntangibleMatter
Copy link
Contributor

Is there any kind of precedent for this kind of syntax for "ignorable" columns in CSV?

In a lot of games' .csv translations that you can find, especially indie games, there are columns which use some sort of syntax to mark them as "ignored". There's no standard per se, but there is a precedent.

@akien-mga
Copy link
Member

Alright, let's give it a try. This needs to be documented in https://docs.godotengine.org/en/latest/tutorials/assets_pipeline/importing_translations.html otherwise most users won't know the feature exists.

@akien-mga akien-mga merged commit c187d65 into godotengine:master Feb 20, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

IntangibleMatter added a commit to IntangibleMatter/godot-docs that referenced this pull request Feb 20, 2024
@jsjtxietian jsjtxietian deleted the Add-ignorable-columns-to-translation-CSVs branch February 20, 2024 13:04
jsjtxietian added a commit to jsjtxietian/godot-docs that referenced this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants