Closed Bug 1344852 Opened 8 years ago Closed 8 years ago

Enable flake8 rule W602: "deprecated form of raising exception"

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement
Points:
1

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: paavininanda, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [measurement:client][lang=python])

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1344850 +++ In bug 1332651 we landed flake8 initial support to lint Python files in the Telemetry directory, by disabling all the detected problems. This bug is about enabling the W602, "deprecated form of raising exception": 1) Remove the W602 rule from the .flake8 file in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry 2) Run "./mach lint -l flake8 toolkit/components/telemetry". 3) Fix the reported problems
No longer depends on: 1344850
Blocks: 1344853
No longer blocks: 1344853
Keywords: good-first-bug
Please assign this to me.
Flags: needinfo?(alessio.placitelli)
Attached patch Corrected the way of raiing exceptions. (obsolete) (deleted) — Splinter Review
Please review this.
Flags: needinfo?(alessio.placitelli)
Assignee: nobody → paavininanda
Attachment #8846592 - Flags: review?(alessio.placitelli)
Comment on attachment 8846592 [details] [diff] [review] Corrected the way of raiing exceptions. Review of attachment 8846592 [details] [diff] [review]: ----------------------------------------------------------------- Please change the commit message to be: Bug 1344852: Enable flake8 rule W602: "deprecated form of raising exception". r?dexter ::: toolkit/components/telemetry/histogram_tools.py @@ +289,5 @@ > > invalid = filter(lambda l: len(l) > MAX_LABEL_LENGTH, labels) > if len(invalid) > 0: > + raise ValueError('Label values for %s exceed length limit of %d: %s' % \ > + (name, MAX_LABEL_LENGTH, ', '.join(invalid))) nit: please align this with 'Label above. i.e. move one whitespace to the left @@ +301,5 @@ > pattern = '^[a-z][a-z0-9_]+[a-z0-9]$' > invalid = filter(lambda l: not re.match(pattern, l, re.IGNORECASE), labels) > if len(invalid) > 0: > + raise ValueError('Label values for %s are not matching pattern "%s": %s' % \ > + (name, pattern, ', '.join(invalid))) nit: please align this with 'Label above. i.e. move one whitespace to the left @@ +370,5 @@ > if not key in definition: > continue > if not isinstance(definition[key], key_type): > + raise ValueError(('value for key "{0}" in Histogram "{1}" ' > + 'should be {2}').format(key, name, nice_type_name(key_type))) Let's move the 'should be ...' part on the previous line and keep .format() on the next line. This could become: raise ValueError('value for key "{0}" in Histogram "{1}" should be {2}'\ .format(key, name, nice_type_name(key_type)) With .format aligned to the 'value @@ +377,5 @@ > if not key in definition: > continue > if not all(isinstance(x, key_type) for x in definition[key]): > + raise ValueError(('all values for list "{0}" in Histogram "{1}" ' > + 'should be {2}').format(key, name, nice_type_name(key_type))) Same as above. ::: toolkit/components/telemetry/parse_events.py @@ +141,5 @@ > (identifier, value, field, max_length)) > # Regex check. > if regex and not re.match(regex, value): > + raise ValueError('%s: string value "%s" for %s is not matching pattern "%s"' % \ > + (identifier, value, field, regex)) Please align (identifier, ...) to the beginning of the '%s on the line above. @@ +170,5 @@ > rcc = definition.get(rcc_key, 'opt-in') > allowed_rcc = ["opt-in", "opt-out"] > if not rcc in allowed_rcc: > + raise ValueError("%s: value for %s should be one of: %s" %\ > + (self.identifier, rcc_key, ", ".join(allowed_rcc))) Same as above @@ +182,5 @@ > # Check extra_keys. > extra_keys = definition.get('extra_keys', {}) > if len(extra_keys.keys()) > MAX_EXTRA_KEYS_COUNT: > + raise ValueError("%s: number of extra_keys exceeds limit %d" %\ > + (self.identifier, MAX_EXTRA_KEYS_COUNT)) Same as above @@ +191,5 @@ > > # Check expiry. > if not 'expiry_version' in definition and not 'expiry_date' in definition: > + raise KeyError("%s: event is missing an expiration - either expiry_version or expiry_date is required" %\ > + (self.identifier)) Same as above @@ +196,5 @@ > expiry_date = definition.get('expiry_date') > if expiry_date and isinstance(expiry_date, basestring) and expiry_date != 'never': > if not re.match(DATE_PATTERN, expiry_date): > + raise ValueError("%s: event has invalid expiry_date, it should be either 'never' or match this format: %s" %\ > + (self.identifier, DATE_PATTERN)) Same as above
Attachment #8846592 - Flags: review?(alessio.placitelli)
Attached patch Deprecated form of raising exception removed (obsolete) (deleted) — Splinter Review
Attachment #8846592 - Attachment is obsolete: true
Attachment #8846754 - Flags: review?(gfritzsche)
Attachment #8846754 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Comment on attachment 8846754 [details] [diff] [review] Deprecated form of raising exception removed Review of attachment 8846754 [details] [diff] [review]: ----------------------------------------------------------------- Paavini, it looks like this patch is identical to the previous one, but with the correct commit message. It's missing the changes suggested in comment 3. Did you forget to update the patch?
Attachment #8846754 - Flags: review?(alessio.placitelli)
Attached patch bug1344852.patch (obsolete) (deleted) — Splinter Review
Made the changes now.
Attachment #8846754 - Attachment is obsolete: true
Attachment #8847048 - Flags: review?(alessio.placitelli)
Comment on attachment 8847048 [details] [diff] [review] bug1344852.patch Paavini, it looks like this patch does not apply cleanly on the latest mozilla-central. From the file, it seems like you've created a new, different patch that is stacked on the one you previously attached. You should be creating only one patch, adding changes to it and then attaching that patch file :)
Flags: needinfo?(paavininanda)
Attachment #8847048 - Flags: review?(alessio.placitelli)
Attached patch bug1344852.patch (deleted) — Splinter Review
Attachment #8847048 - Attachment is obsolete: true
Flags: needinfo?(paavininanda)
Attachment #8847561 - Flags: review?(alessio.placitelli)
Comment on attachment 8847561 [details] [diff] [review] bug1344852.patch Review of attachment 8847561 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks.
Attachment #8847561 - Flags: review?(alessio.placitelli) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6804841ffca5bcdc8785735ef93b516249828497 Bug 1344852: Enable flake8 rule W602: "deprecated form of raising exception". r=dexter
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: