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

Submitter:  Philip Withnall <drbob>
Submitted:  Tue 27 Jan 2015 10:50:34 AM UTC
   
 
Category:  Programmer tools Severity:  3 - Normal
Item Group:  None Status:  Fix Released
Privacy:  Public Assigned to:  ueno
Open/Closed:  Closed

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>
Group administrator
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>
Group administrator
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>
Group 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>
Group 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>
Group administrator
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>
Group administrator
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>
Group 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:


$ xgettext -Wunicode-quote -Wno-space-before-ellipsis -k_ -o- x-vala.c
x-vala.c:176:0: Unicode double quote should be used instead of ASCII
xgettext: found 1 fatal error


(file #32936)

Daiki Ueno <ueno>
Group administrator
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>
Group 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>
Group administrator
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>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-01-04 haible StatusFixed Fix Released
    2016-11-27 haible CategoryNone Programmer tools
    2016-05-27 ueno StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2015-01-29 ueno Attached File- Added 0001-xgettext-Support-message-syntax-checks.patch, #32936
        StatusNone In Progress
        Assigned toNone ueno
    2015-01-27 piotrdrag Carbon-Copy- Added piotrdrag

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code