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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

+++ 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

Comment 1

[Paavini Nanda]

Please assign this to me.

Comment 2

[Paavini Nanda]

Attachment: Corrected the way of raiing exceptions.

Please review this.

Comment 3

[Alessio Placitelli [:Dexter]]

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

Comment 4

[Paavini Nanda]

Attachment: Deprecated form of raising exception removed

Comment 5

[Alessio Placitelli [:Dexter]]

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?

Comment 6

[Paavini Nanda]

Attachment: bug1344852.patch

Made the changes now.

Comment 7

[Alessio Placitelli [:Dexter]]

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 :)

Comment 8

[Paavini Nanda]

Attachment: bug1344852.patch

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8847561 [details] [diff] [review]
bug1344852.patch

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

This looks good, thanks.

Comment 10

[Alessio Placitelli [:Dexter]]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6804841ffca5