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

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: paavininanda, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla55
good-first-bug
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
Created attachment 8846592 [details] [diff] [review]
Corrected the way of raiing exceptions.

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
Created attachment 8846754 [details] [diff] [review]
Deprecated form of raising exception removed
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
Created attachment 8847048 [details] [diff] [review]
bug1344852.patch

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
Created attachment 8847561 [details] [diff] [review]
bug1344852.patch
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
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.