bugGNU gettext - Bugs: bug #44098, Check strings for common problems

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #44098: Check strings for common problems

Submitted by:  Philip Withnall <drbob>
Submitted on:  Tue 27 Jan 2015 10:50:34 AM UTC  
 
Category: Programmer toolsSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: Daiki Ueno <ueno>
Open/Closed: Closed

(Jump to the original submission Jump to the original submission)

Fri 27 May 2016 09:09:32 AM UTC, comment #10:

This was added in 0.19.5.

Daiki Ueno <ueno>
Project AdministratorIn charge of this item.
Tue 03 Mar 2015 10:09:33 AM UTC, comment #9:

Sorry for the delay. I've merged the current patch:
http://git.savannah.gnu.org/cgit/gettext.git/commit/?id=d9fc3d2a

Changes from v2 are:
- The shorthand option -W has been removed, since it could potentially mean a negation of the long option --check
- Multi-sentence is now supported for ellipsis checks. The sentence recognition logic might not be sufficient, as it's largely based on Emacs's forward-sentence:
http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-tools/src/sentence.c
- New option --sentence-end is added (to require two spaces after a period, do --sentence-end=double-space)

Anyway I'll keep this issue open for a while, until the design/implementation settles down. Further comments would be appreciated.

Daiki Ueno <ueno>
Project AdministratorIn charge of this item.
Fri 06 Feb 2015 01:20:31 PM UTC, comment #8:

> • Not enforce a rule on the developer which is tied to a particular style guide.


Actually, I’ve changed my mind on that one. As long as the rule must apply to every string in a project, it should be OK. So the two-spaces-after-period check sounds good to me (I just personally wouldn’t enable it).

Philip Withnall <drbob>
Project Member
Fri 06 Feb 2015 01:18:17 PM UTC, comment #7:

Thanks for the updated patch. I’ve taken a quick look over it and it looks good to me, though I’m no gettext reviewer.

I think support for multi-sentence strings is important, as a lot of the strings I can think of which would contain quotation marks are likely to be multi-sentence, such as the message in a dialogue box.

> Here are some I have in mind:
> - check if a message is/isn't ends with "."
> - check if a message is/isn't capitalized
> - suggest two spaces after "." for multi-sentence strings


I don’t think these will work as checks. I believe checks should:
• Be an absolute rule: there should be no situations where a string should legitimately fail the check.
• Not enforce a rule on the developer which is tied to a particular style guide.
• Catch common errors; it’s not worth trying to catch uncommon ones.

Plenty of messages (e.g. button labels, menu entries) should not be full sentences. Various messages should not be capitalised either (for example, messages starting with ‘e.g.’ or giving search keywords in .desktop files). The style of putting two spaces after a period dates from typewriters and monospace fonts, and is generally irrelevant with modern fonts.

I’ll send an e-mail to the GNOME i18n list asking for feedback and suggestions for additional checks to perform.

Philip Withnall <drbob>
Project Member
Wed 04 Feb 2015 09:53:53 AM UTC, comment #6:

To facilitate reviewing, I've posted the 2nd patch to the mailing-list (we should probably migrate to debbugs this year).
https://lists.gnu.org/archive/html/bug-gettext/2015-02/msg00001.html

Changes from v1 are:
- recognize "xgettext: no-*-check" comment in source code
- add test case
- refactor quoted segment scanning into a separate file
which was done in a separate commit:
http://git.sv.gnu.org/cgit/gettext.git/commit/?id=01658057
- refactor syntax check function invocation

I'm a bit reluctant to support multi-sentence strings at the moment, since the original tools (po/Rules-quot and the indicator-applet's Makefile.am snippets) only consider single-sentence strings. So that part has not changed since v1.

Also, suppression of checks with a command-line option is not implemented (yet) for implementation reasons (currently the --flag option only accepts *-format flags).

Anyway, further reviewing and suggestions for further checks would be appreciated.

Here are some I have in mind:
- check if a message is/isn't ends with "."
- check if a message is/isn't capitalized
- suggest two spaces after "." for multi-sentence strings

Daiki Ueno <ueno>
Project AdministratorIn charge of this item.
Fri 30 Jan 2015 01:19:09 PM UTC, comment #5:

Thanks for the review, they are all good points. I'll soon update the patch with tests. On error suppression, maybe we could reuse flags (the --flag option or "xgettext: ..." comment in the source code), though we should probaly name the warnings consistently like gcc's -W, e.g. no-*-check.

Daiki Ueno <ueno>
Project AdministratorIn charge of this item.
Thu 29 Jan 2015 10:42:40 AM UTC, comment #4:

check_unicode_ellipsis(): this seems to check for ellipses only at the end of lines, whereas they can actually appear anywhere in a multi-sentence translatable string.

check_no_space_before_ellipsis(): same.

check_unicode_quote() should handle the case of a closing quotation mark followed by punctuation, rather than just followed by a space.

syntax_check_message(): this could be refactored as an array of checks to perform, which would make it easier to add new checks in future:

struct {
int enable_flag;
CheckFunc func;
} check_funcs[] = {
{ CHECK_UNICODE_ELLIPSIS, check_unicode_ellipsis },
{ CHECK_NO_SPACE_BEFORE_ELLIPSIS, check_no_space_before_ellipsis },

};

for (i = 0; i < sizeof (check_funcs) / sizeof (*check_funcs); i++) {
if (!(checks & check_funcs[i].enable_flag))
continue;

seen_errors += check_funcs[i].func (mp, mp->msgid, msgid_pos, checks);
if (mp->msgid_plural)
seen_errors += check_funcs[i].func (mp, mp->msgid_plural, msgid_pos, checks);
}

Overall, this looks great to me, thanks a lot! One thing I think it’s missing is a way of suppressing errors, so that the warnings can be enabled unconditionally, but false positives for strings which are actually correct can be suppressed. How about a per-string tag in the .pot file comments, similar to the c-format tag — something like ‘suppress-warnings=no-space-before-ellipsis,unicode-quote’?

Philip Withnall <drbob>
Project Member
Thu 29 Jan 2015 08:33:53 AM UTC, comment #3:

> I suggest the ultimate goal is to get the .pot file checked in `make check -C po`.


In that case, xgettext would be a better place to implement the feature, because its invocation can be controlled through either gettext's XGETTEXT_OPTIONS make variable or intltool's XGETTEXT_ARGS envvar.

I'm attaching a tentative patch. It can be used like:

(file #32936)

Daiki Ueno <ueno>
Project AdministratorIn charge of this item.
Wed 28 Jan 2015 08:56:10 AM UTC, comment #2:

That sounds reasonable to me. I suggest the ultimate goal is to get the .pot file checked in `make check -C po`.

Philip Withnall <drbob>
Project Member
Wed 28 Jan 2015 02:58:04 AM UTC, comment #1:

Thanks, that's an interesting idea, but I don't think msgfmt is the right place to add those checks. The command is supposed to take a PO file rather than a POT file as its input, and it would be annoying if the warnings were issued when processing every PO file.

On the other hand, this sort of checks look analogous to syntax check rules in Gnulib's maintainer-makefile, which some projects include in their repositories to ease maintenance tasks (but not in distribution tarballs):
http://git.savannah.gnu.org/cgit/gnulib.git/tree/top/maint.mk#n147

So, as a first step we can provide those checks as a built-in subcommand of msgexec, so they can be invoked from such a maintainer-makefiles. Is this approach feasible to the GNOME's use of gettext?

Daiki Ueno <ueno>
Project AdministratorIn charge of this item.
Tue 27 Jan 2015 10:50:34 AM UTC, original submission:

It would be interesting if gettext could optionally run some tests on strings when it compiles a catalogue.

There are some here, written as a Makefile, but they could be ported to gettext easily enough:

http://bazaar.launchpad.net/~indicator-applet-developers/indicator-session/trunk.13.04/view/head:/tests/Makefile.am.strings

For example, they check that a Unicode ellipsis character has been used instead of three periods in a row, that ASCII quotation marks aren’t used, and that there are no spaces before ellipses.

These rules are somewhat style-specific, so should not be enabled for all projects. Perhaps they could be enabled individually as a series of checker options?

e.g.
msgfmt -Wunicode-ellipses -Wspace-before-ellipsis -Wascii-quotes …

This was originally a bug against gnome-common, but we decided it wasn’t the right place to implement it: https://bugzilla.gnome.org/show_bug.cgi?id=689602.

Philip Withnall <drbob>
Project Member

 

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by haible (Updated the item)
  • -unavailable- added by ueno (Posted a comment)
  • -unavailable- added by piotrdrag
  • -unavailable- added by drbob (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 27 Nov 2016 11:58:41 AM UTChaibleCategoryNone=>Programmer tools
    Fri 27 May 2016 09:09:32 AM UTCuenoStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Thu 29 Jan 2015 08:33:53 AM UTCuenoAttached File-=>Added 0001-xgettext-Support-message-syntax-checks.patch, #32936
      StatusNone=>In Progress
      Assigned toNone=>ueno
    Tue 27 Jan 2015 01:42:41 PM UTCpiotrdragCarbon-Copy-=>Added piotrdrag

    Back to the top


    Powered by Savane 3.1-cleanup1