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

Please assign this to me.

Please review this.

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

Made the changes now.

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 on attachment 8847561 [details] [diff] [review] bug1344852.patch Review of attachment 8847561 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks.

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