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

Change: [strgen] 'warnings' for translations are now 'infos' unless invoked with -w or -t #9406

Merged
merged 1 commit into from Jun 29, 2021

Conversation

@glx22
Copy link
Contributor

@glx22 glx22 commented Jun 29, 2021

Motivation / Problem

A change in gcc-problem-matcher made it pickup strgen warnings for translations. These are usually handled by eints later.

Description

Use info for translation warnings, unless -w or -t switches are used.
This is done by abusing _show_todo.
Tested on #9017 (see output here).
I also tested locally to be sure english.txt still always generate warning and not info.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
Copy link
Member

@TrueBrain TrueBrain left a comment

This commit message is way to specific :P It is okay to use extra lines in the commit message, like:

Change: [strgen] Change warnings for translations into infos

Unless invoked with -w ("show warnings") or -t ("show todos")

Eints normally fixes the warnings after a Pull Request, so it is not
really useful information for the developer to see as a warning.

At least, I am guessing that is what -w and -t do, please feel free to correct where needed :P

Loading

fprintf(stderr, LINE_NUM_FMT("warning"), _file, _cur_line, buf);
} else {
fprintf(stderr, LINE_NUM_FMT("info"), _file, _cur_line, buf);
}
Copy link
Member

@TrueBrain TrueBrain Jun 29, 2021

Choose a reason for hiding this comment

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

why would the classification change depending on a setting? I am not sure I follow that logic. For who will it become a warning that doesn't think info is what he is looking for?

Loading

Copy link
Contributor Author

@glx22 glx22 Jun 29, 2021

Choose a reason for hiding this comment

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

I'm not sure to fully understand your question.
-t sets bit 0 of _show_todo, -w sets bit 1, and now english.txt sets bit 2.
So if any bit is set it's a warning, else it's an info.

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 29, 2021

Choose a reason for hiding this comment

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

No, I am wondering why this if-statement exists in the first place. Why would it be a "warning" in some cases, and "info" in others. That always feels a bit weird, and often tells there is something else being a bit off. I haven't checked how other places do it. I just don't get why the log-level would change depending on -t for example.

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 29, 2021

Choose a reason for hiding this comment

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

Okay, a bit closer look, I do not think this is the right approach. There are plenty of warnings that really are warnings, and never infos. Like invalid UTF-8, for example. What if we just add a new command like strgen_translation_warning, in that case I could understand this if-statement a lot more :)

Edit: or better yet, just don't output anything if -w and -t aren't set for translation errors? :D

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 29, 2021

Choose a reason for hiding this comment

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

Nevermind all this, it is already messy as fuck. So it is not fair to ask from you to fix "everything" just to fix a bug :P So lets just do this.

Loading

Unless invoked with -w, --warning ("print a warning for any untranslated strings") or -t, --todo ("replace any untranslated strings with '<TODO>'").

Eints normally fixes the warnings after a Pull Request, so it is not really useful information for the developer to see as a warning.
@glx22 glx22 merged commit b6fb020 into OpenTTD:master Jun 29, 2021
15 checks passed
Loading
@glx22 glx22 deleted the strgen branch Jun 29, 2021
Taschi120 added a commit to Taschi120/OpenTTD that referenced this issue Jul 11, 2021
…#9406)

Unless invoked with -w, --warning ("print a warning for any untranslated strings") or -t, --todo ("replace any untranslated strings with '<TODO>'").

Eints normally fixes the warnings after a Pull Request, so it is not really useful information for the developer to see as a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants