Closed Bug 1024975 Opened 5 years ago Closed 5 years ago
[browser] Ignore searchplugins/metrolist
.txt in filter .py
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.
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 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+
Agreed, that was my first version. I think it also makes clearer that we always return at that point in the cycle.
Carrying over f+ from previous patch, asking Gavin for official review.
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+
Thanks Gavin, I'll be happy to stop nagging you for this :-)
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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+
You need to log in before you can comment on or make changes to this bug.