Closed Bug 697429 Opened 10 years ago Closed 3 years ago

Bring filter.py up to date for compare locales

Categories

(Thunderbird :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: standard8, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch The fix (obsolete) — Splinter Review
Thunderbird's filter.py has gotten a little out of date and needs a bit of work, namely:

- We shouldn't exclude checking some strings in plugins.dtd, especially as we want them now.
- Match the rest of the file to the same style as Firefox's filter.py so that we can easily translate changes across from one file to the other.
Attachment #569670 - Flags: review?(l10n)
Flags: in-testsuite-
Comment on attachment 569670 [details] [diff] [review]
The fix

Review of attachment 569670 [details] [diff] [review]:
-----------------------------------------------------------------

We haven't done this for any of firefox etc, but if we're changing big, we should change for the better. A good example is the 1.9.2 one, which uses text flags instead of fancy bools, see http://hg.mozilla.org/releases/mozilla-1.9.2/file/default/browser/locales/filter.py.

Can we just go that route instead?
Attachment #569670 - Flags: review?(l10n) → review-
Attached patch The fix v2Splinter Review
Similar patch to the previous but uses "ignore" and "error" in the return values.
Attachment #569670 - Attachment is obsolete: true
Attachment #579633 - Flags: review?(l10n)
Comment on attachment 579633 [details] [diff] [review]
The fix v2

Review of attachment 579633 [details] [diff] [review]:
-----------------------------------------------------------------

This looks much better than the true/false mess, but there are some comments and logic that could even be stronger, thus an r-.

::: mail/locales/filter.py
@@ +18,3 @@
>    if mod == "extensions/spellcheck":
> +    # l10n ships en-US dictionary or something, do compare
> +    return "error"

The comment here is a tad confusing. What this is doing is that if we had a .dtd or .properties file in en-US and l10n, it'd actually compare. Really just a safeguard.

See also the comment below.

@@ +26,5 @@
> +  if path != "chrome/messenger-region/region.properties":
> +    # only region.properties exceptions remain, compare all others
> +    return "error"
> +  
> +  return (re.match(r"browser\.search\.order\.[1-9]", entity)) and "ignore" or "error"

I'd suggest to invert the logic, and have all exceptions being in explicit conditions.

Something like (with 4space indention, too):

if mod == "mail":
  if path == "defines.inc":
    return (entity == "MOZ_LANGPACK_CONTRIBUTORS") and "ignore" or "error"
  elif path == "chrome/messenger-region/region.properties":
    return (re.match(r"browser\.search\.order\.[1-9]", entity)) and "ignore" or "error"

return "error"
Attachment #579633 - Flags: review?(l10n) → review-
Assignee: mbanner → nobody
Do we/Thunderbird still need this?
Flags: needinfo?(philipp)
Flags: needinfo?(l10n)
Can't speak to the initial comment here, really. I'd suspect yes, though.
Flags: needinfo?(l10n)

Let's close this and create a new bug if something breaks.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(philipp)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.