[browser] Ignore searchplugins/metrolist.txt in filter.py

RESOLVED FIXED in Firefox 32

Status

Firefox for Metro
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: flod, Assigned: flod)

Tracking

unspecified
Firefox 33

Firefox Tracking Flags

(firefox32 fixed, firefox33 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Bug 1018257 removed checks on Metro localization files, but we're still requiring searchplugins/metrolist.txt, that prevents us from deleting the file in l10n repositories.
(Assignee)

Comment 1

3 years ago
Created attachment 8439841 [details] [diff] [review]
bug1024975.patch

I'm open to suggestion for the Python syntax: I find it hard to read in short format + 2 conditions, I could also ignore "else:" and just return "error" after checking for files to ignore in /browser
Attachment #8439841 - Flags: feedback?(l10n)

Comment 2

3 years ago
Comment on attachment 8439841 [details] [diff] [review]
bug1024975.patch

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

Yeah, I think you're right that restructuring makes this more readable.

I'd take it one step further, and not use an else:, but
if blah: return "ignore"
if foo: return "ignore"
return "error"

::: browser/locales/filter.py
@@ +23,5 @@
> +    if (re.match(r"searchplugins\/.+\.xml", path) or
> +        path == "searchplugins/metrolist.txt"):
> +      return "ignore"
> +    else:
> +      return "error"

I'd drop the else: and just close the 
if not entity:
  .
  .
  .
  return "error"
Attachment #8439841 - Flags: feedback?(l10n) → feedback+
(Assignee)

Comment 3

3 years ago
Agreed, that was my first version. I think it also makes clearer that we always return at that point in the cycle.
(Assignee)

Comment 4

3 years ago
Created attachment 8439847 [details] [diff] [review]
bug1024975v2.patch

Carrying over f+ from previous patch, asking Gavin for official review.
Attachment #8439841 - Attachment is obsolete: true
Attachment #8439847 - Flags: review?(gavin.sharp)
Comment on attachment 8439847 [details] [diff] [review]
bug1024975v2.patch

I hereby declare that :Pike and :flod are official browser peers for the purposes of reviewing browser/locales/filter.py. Please feel free to review each others work here without any additional review. As mentioned previously, I am happy to sign-off on any policy changes if you deem it necessary. (As straightforward followup to bug 1018257, this shouldn't require any additional sign-off IMO.)
Attachment #8439847 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 6

3 years ago
Thanks Gavin, I'll be happy to stop nagging you for this :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/49f7863e004f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/49f7863e004f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
(Assignee)

Comment 9

3 years ago
Comment on attachment 8439847 [details] [diff] [review]
bug1024975v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: localizers won't be able to remove all metro files on mozilla-aurora, since only a part of the checks has been removed before last merge day (bug 1018257).
Testing completed (on m-c, etc.): yes on m-c
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none
Attachment #8439847 - Flags: approval-mozilla-aurora?
Comment on attachment 8439847 [details] [diff] [review]
bug1024975v2.patch

Aurora approval granted.
Attachment #8439847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0e9e8004857
status-firefox32: --- → fixed
status-firefox33: --- → fixed
You need to log in before you can comment on or make changes to this bug.