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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: paavininanda, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla55
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client][lang=python])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

2 years ago
+++ 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
Reporter

Updated

2 years ago
No longer depends on: 1344850
Reporter

Updated

2 years ago
Blocks: 1344853
Reporter

Updated

2 years ago
No longer blocks: 1344853
Reporter

Updated

2 years ago
Keywords: good-first-bug
Assignee

Comment 1

2 years ago
Please assign this to me.
Flags: needinfo?(alessio.placitelli)
Assignee

Comment 2

2 years ago
Please review this.
Flags: needinfo?(alessio.placitelli)
Reporter

Updated

2 years ago
Assignee: nobody → paavininanda
Reporter

Updated

2 years ago
Attachment #8846592 - Flags: review?(alessio.placitelli)
Reporter

Comment 3

2 years ago
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)
Assignee

Comment 4

2 years ago
Attachment #8846592 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8846754 - Flags: review?(gfritzsche)
Attachment #8846754 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Reporter

Comment 5

2 years ago
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)
Assignee

Comment 6

2 years ago
Posted patch bug1344852.patch (obsolete) — Splinter Review
Made the changes now.
Attachment #8846754 - Attachment is obsolete: true
Attachment #8847048 - Flags: review?(alessio.placitelli)
Reporter

Comment 7

2 years ago
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)
Assignee

Comment 8

2 years ago
Attachment #8847048 - Attachment is obsolete: true
Flags: needinfo?(paavininanda)
Assignee

Updated

2 years ago
Attachment #8847561 - Flags: review?(alessio.placitelli)
Reporter

Comment 9

2 years ago
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+
Reporter

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6804841ffca5bcdc8785735ef93b516249828497
Bug 1344852: Enable flake8 rule W602: "deprecated form of raising exception". r=dexter

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6804841ffca5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.