+++ This bug was initially created as a clone of Bug #1344833 +++ 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 E501 "line too long (113 > 99 characters)" 1) Remove the E501 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

Hi, I am going to work on this and remove all the rules in .flake8 that need to be removed.

Comment on attachment 8848653 [details] Bug 1344834 - Enable flake8 rule E501 and and fixes errors.; https://reviewboard.mozilla.org/r/121558/#review124860 This is a good start, thank you for the patch and sorry for the delay. I'd strongly suggest you to rebase your patch against the latest version of the files, as many things have changed and some stuff will need to be fixed. Please, also make sure to run the flake8 tests before pushing the updated patch and the normal telemetry tests: ./mach test toolkit/components/telemetry ::: commit-message-23a4b:1 (Diff revision 1) > +Bug 1344834 - Enable flake8 rule E501 and and fixes errors.; r?Dexter Please remove the additional ; after the dot before "r?Dexter" ::: toolkit/components/telemetry/gen-histogram-data.py:103 (Diff revision 1) > n_buckets = histogram.n_buckets() > static_assert(output, "%s < %s" % (low, high), "low >= high for %s" % name) > static_assert(output, "%s > 2" % n_buckets, "Not enough values for %s" % name) > static_assert(output, "%s >= 1" % low, "Incorrect low value for %s" % name) > static_assert(output, "%s > %s" % (high, n_buckets), > - "high must be > number of buckets for %s; you may want an enumerated histogram" % name) > + "high must be > number of buckets for %s;" Please att a whitespace after ";" on the first part of the string. ::: toolkit/components/telemetry/gen-histogram-enum.py:81 (Diff revision 1) > print(" HistogramDUMMY2,", file=output) > print(" HistogramLastUseCounter = HistogramDUMMY2 - 1,", file=output) > > print(" HistogramCount,", file=output) > if seen_use_counters: > - print(" HistogramUseCounterCount = HistogramLastUseCounter - HistogramFirstUseCounter + 1", file=output) > + print(" HistogramUseCounterCount = HistogramLastUseCounter -" Please add a whitespace after the dash otherwise the full string will become "HistogramLastUseCounter -HistogramFirstUseCounter + 1". ::: toolkit/components/telemetry/gen-histogram-enum.py:103 (Diff revision 1) > for name, _, _ in enums: > print("template<> struct IsCategoricalLabelEnum<%s> : TrueType {};" % name, file=output) > > print("\ntemplate<class T> struct CategoricalLabelId {};", file=output) > for name, _, id in enums: > - print("template<> struct CategoricalLabelId<%s> : IntegralConstant<uint32_t, %s> {};" % (name, id), file=output) > + print("template<> struct CategoricalLabelId<%s> :" Please add a whitespace after : so that the first part of the string becomes "template<> struct CategoricalLabelId<%s> : " ::: toolkit/components/telemetry/histogram_tools.py:85 (Diff revision 1) > > whitelists = None > try: > - whitelist_path = os.path.join(os.path.abspath(os.path.realpath(os.path.dirname(__file__))), 'histogram-whitelists.json') > + whitelist_path = os.path.join(os.path.abspath( > + os.path.realpath(os.path.dirname(__file__))), > + 'histogram-whitelists.json') Does 'histogram-whitelists.json' need to be on a separate line? ::: toolkit/components/telemetry/histogram_tools.py:95 (Diff revision 1) > whitelists[name] = set(whitelist) > except ValueError, e: > raise BaseException, 'error parsing whitelist (%s)' % whitelist_path > except IOError: > whitelists = None > - print 'Unable to parse whitelist (%s). Assuming all histograms are acceptable.' % whitelist_path > + print 'Unable to parse whitelist (%s).' If you wrap the string in parenthesis, you would simply need to put whitelist_path on a new line. So this line becomes: print('Unable to parse whitelist (%s). Assuming all histograms are acceptable.' % whitelist_path) ::: toolkit/components/telemetry/histogram_tools.py:327 (Diff revision 1) > for field in ['alert_emails', 'bug_numbers']: > if field not in definition and name not in whitelists[field]: > raise KeyError, 'New histogram "%s" must have a %s field.' % (name, field) > if field in definition and name in whitelists[field]: > - msg = 'Should remove histogram "%s" from the whitelist for "%s" in histogram-whitelists.json' > + msg = 'Should remove histogram "%s" from the whitelist' > + 'for "%s" in histogram-whitelists.json' Please align this with the first part of the string above. Also add a whitespace at the end of the previous string. ::: toolkit/components/telemetry/histogram_tools.py:399 (Diff revision 1) > self._low = low > self._high = high > self._n_buckets = n_buckets > if whitelists is not None and self._n_buckets > 100 and type(self._n_buckets) is int: > if self._name not in whitelists['n_buckets']: > - raise KeyError, ('New histogram "%s" is not permitted to have more than 100 buckets. ' > + raise KeyError, ('New histogram "%s" is not permitted' Please use all the space you have up to 99 characters per line. There's no need to break it too much ::: toolkit/components/telemetry/histogram_tools.py:426 (Diff revision 1) > > @staticmethod > def categorical_bucket_parameters(definition): > # Categorical histograms default to 50 buckets to make working with them easier. > - # Otherwise when adding labels later we run into problems with the pipeline not supporting bucket changes. > + # Otherwise when adding labels later we run into problems > + # with the pipeline not supporting bucket changes. Let's take all the space available on the previous line before breaking the line. ::: toolkit/components/telemetry/histogram_tools.py:527 (Diff revision 1) > lower_bound = use_counter_indices[0][0] > upper_bound = use_counter_indices[-1][0] > n_counters = upper_bound - lower_bound + 1 > if n_counters != len(use_counter_indices): > - raise DefinitionException, "use counter histograms must be defined in a contiguous block" > + raise DefinitionException, "use counter histograms" > + "must be defined in a contiguous block" Please align this with the beginning of the previous line and make sure to add a whitespace at the end of the previous string. ::: toolkit/components/telemetry/histogram_tools.py:536 (Diff revision 1) > if whitelists is not None: > all_whitelist_entries = itertools.chain.from_iterable(whitelists.itervalues()) > orphaned = set(all_whitelist_entries) - set(all_histograms.keys()) > if len(orphaned) > 0: > - msg = 'The following entries are orphaned and should be removed from histogram-whitelists.json: %s' > + msg = 'The following entries are orphaned and should' > + 'be removed from histogram-whitelists.json: %s' Please align this string to the beginning of the previous string. ::: toolkit/components/telemetry/parse_events.py:86 (Diff revision 1) > nice_type_name(type(x))) > > # Check types of keys and values in dictionaries. > elif self._kind is dict: > if len(value.keys()) < 1: > - raise ValueError, "%s: failed check for %s - dict should not be empty" % (identifier, key) > + raise ValueError, "%s: failed check for %s" When you rebase your patch, you will only need to put the (identifier, key) part on a new line. You have up to 99 characters. Also, don't forget to add a whitespace at the end of the previous part. ::: toolkit/components/telemetry/parse_events.py:91 (Diff revision 1) > - raise ValueError, "%s: failed check for %s - dict should not be empty" % (identifier, key) > + raise ValueError, "%s: failed check for %s" > + "- dict should not be empty" % (identifier, key) > for x in value.iterkeys(): > if not isinstance(x, self._args[0]): > - raise ValueError, "%s: failed dict type check for %s - expected key type %s, got %s" %\ > + raise ValueError, "%s: failed dict type check for %s -" > + "expected key type %s, got %s" %\ You will need to align this with the first part of the string. ::: toolkit/components/telemetry/parse_events.py:98 (Diff revision 1) > nice_type_name(self._args[0]), > nice_type_name(type(x))) > for k, v in value.iteritems(): > if not isinstance(x, self._args[1]): > - raise ValueError, "%s: failed dict type check for %s - expected value type %s for key %s, got %s" %\ > + raise ValueError, "%s: failed dict type check for %s -" > + "expected value type %s for key %s, got %s" %\ Same here. ::: toolkit/components/telemetry/parse_events.py:186 (Diff revision 1) > # Check record_in_processes. > record_in_processes = definition.get('record_in_processes') > for proc in record_in_processes: > if not utils.is_valid_process_name(proc): > - raise ValueError(self.identifier + ': unknown value in record_in_processes: ' + proc) > + raise ValueError(self.identifier + ': unknown value in' > + 'record_in_processes: ' + proc) No need to split the string here, just move proc on a new line. ::: toolkit/components/telemetry/parse_events.py:201 (Diff revision 1) > regex=IDENTIFIER_PATTERN) > > # 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" %\ > + raise KeyError, "%s: event is missing an expiration -" > + "either expiry_version or expiry_date is required" %\ Align this to the beginning of the previous string and make sure to add a whitespace at the end of the first part. ::: toolkit/components/telemetry/parse_events.py:207 (Diff revision 1) > (self.identifier) > 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" %\ > + raise ValueError, "%s: event has invalid expiry_date," > + "it should be either 'never' or match this format: %s" %\ Alignment + whitespace :) ::: toolkit/components/telemetry/parse_events.py:213 (Diff revision 1) > (self.identifier, DATE_PATTERN) > # Parse into date. > definition['expiry_date'] = datetime.datetime.strptime(expiry_date, '%Y-%m-%d') > > # Finish setup. > - definition['expiry_version'] = utils.add_expiration_postfix(definition.get('expiry_version', 'never')) > + definition['expiry_version'] = utils.add_expiration_postfix( Just break the line after definition['expiry_version'] = All you need is a \ ::: toolkit/components/telemetry/parse_scalars.py:59 (Diff revision 1) > def check_name(name, error_msg_prefix, allowed_char_regexp): > # Check if we only have the allowed characters. > chars_regxp = r'^[a-zA-Z0-9' + allowed_char_regexp + r']+$' > if not re.search(chars_regxp, name): > - raise ValueError(error_msg_prefix + " name must be alpha-numeric. Got: '{}'".format(name)) > + raise ValueError(error_msg_prefix + " name must be" > + "alpha-numeric. Got: '{}'".format(name)) No need to break the string here. Just move .format(..) on a new line. ::: toolkit/components/telemetry/shared_telemetry_utils.py:94 (Diff revision 1) > > f.write("const char %s[] = {\n" % name) > for (string, offset) in entries: > if "*/" in string: > - raise ValueError, "String in string table contains unexpected sequence '*/': %s" % string > + raise ValueError, "String in string table contains" > + "unexpected sequence '*/': %s" % string Once you rebase, you can simply move string on a new line.

gypsy, are you still interested in working on this bug?

Avikalpa, would you be interested in taking this one?

Ok.

Comment on attachment 8862825 [details] [diff] [review] 1344834.patch Review of attachment 8862825 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks for your patch! This still needs a few tweaks. Please address the comments below and make sure to use the full available line width where possible. Would you please also rebase this patch? Some changes landed recently that changed a few error messages. One last thing: before uploading the updated patch, would you mind testing using > ./mach test toolkit/components/telemetry and > ./mach lint -l flake8 toolkit/components/telemetry Thanks you! ::: toolkit/components/telemetry/.flake8 @@ +1,3 @@ > [flake8] > # See http://pep8.readthedocs.io/en/latest/intro.html#configuration > +ignore = We have no rule left, you can remove this line. ::: toolkit/components/telemetry/gen-histogram-data.py @@ +98,5 @@ > n_buckets = histogram.n_buckets() > static_assert(output, "%s < %s" % (low, high), "low >= high for %s" % name) > static_assert(output, "%s > 2" % n_buckets, "Not enough values for %s" % name) > static_assert(output, "%s >= 1" % low, "Incorrect low value for %s" % name) > + static_assert( There's no need to move |output, "%s > %s" % (high, n_buckets),| on a new line, just break the string below. For example: static_assert(output, "%s > %s" % (high, n_buckets), ("high must be > number of buckets for %s; " "you may want an enumerated histogram") % name) ::: toolkit/components/telemetry/gen-histogram-enum.py @@ +78,5 @@ > > print(" HistogramCount,", file=output) > if seen_use_counters: > + print( > + " HistogramUseCounterCount = HistogramLastUseCounter - HistogramFirstUseCounter + 1", No need to break the line after |print(| ::: toolkit/components/telemetry/histogram_tools.py @@ +91,5 @@ > raise BaseException('error parsing whitelist (%s)' % whitelist_path) > except IOError: > whitelists = None > + print('Unable to parse whitelist (%s).' > + ' Assuming all histograms are acceptable.' % whitelist_path) Since you've added parentheses, you can simply break the line after "%" ::: toolkit/components/telemetry/parse_events.py @@ +187,5 @@ > # Check record_in_processes. > record_in_processes = definition.get('record_in_processes') > for proc in record_in_processes: > if not utils.is_valid_process_name(proc): > + raise ValueError(self.identifier + You can use the full line up to 99 chars: no need to break the line after |self.identifier +| @@ +204,5 @@ > > # Check expiry. > if 'expiry_version' not in definition and 'expiry_date' not in definition: > + raise KeyError("%s: event is missing an expiration - either expiry_version or" > + " expiry_date is required" % self.identifier) Are you sure this is printing as expected? @@ +209,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) Why did you drop the parentheses here? Let's not do that, unless there's a compelling reason to do so @@ +214,5 @@ > # Parse into date. > definition['expiry_date'] = datetime.datetime.strptime(expiry_date, '%Y-%m-%d') > > # Finish setup. > + definition['expiry_version'] = utils.add_expiration_postfix( Let's break this line by adding a "\" character right after the |definition['expiry_version'] =| and moving the rest of the code to a new line (properly indented) ::: toolkit/components/telemetry/parse_scalars.py @@ +112,5 @@ > # Checks that all the required fields are available. > missing_fields = [f for f in REQUIRED_FIELDS.keys() if f not in definition] > if len(missing_fields) > 0: > + raise KeyError( > + self._name + ' - missing required fields: ' + ', '.join(missing_fields) + Don't break the line after |KeyError(|. Break it after |' - missing required fields: ' +| and try to use all the space up to 99 chars. ::: toolkit/components/telemetry/shared_telemetry_utils.py @@ +90,5 @@ > f.write("const char %s[] = {\n" % name) > for (string, offset) in entries: > if "*/" in string: > + raise ValueError( > + "String in string table contains unexpected sequence '*/': %s" % string) Let's break the line after "%", before string

(In reply to Alessio Placitelli [:Dexter] from comment #8) > One last thing: before uploading the updated patch, would you mind testing using > ./mach test toolkit/components/telemetry Not at all! Here is the result of the test. Summary ======= Ran 30 tests Expected results: 30 Unexpected results: 0 OK > ./mach lint -l flake8 toolkit/components/telemetry All OK. > @@ +204,5 @@ > > # Check expiry. > if 'expiry_version' not in definition and 'expiry_date' not in definition: > + raise KeyError("%s: event is missing an expiration - either expiry_version or" > + " expiry_date is required" % self.identifier) > > Are you sure this is printing as expected? I'm not sure what you're talking about. Would it be possible for you to elaborate? Are you referring to the string cutting? It's all right according to: - https://docs.python.org/2.7/reference/lexical_analysis.html#implicit-line-joining - https://docs.python.org/3/reference/lexical_analysis.html#implicit-line-joining If I'm mistaken, my apologies. -- PS: All stuff worked as expected and I rebased as requested.

Comment on attachment 8863959 [details] [diff] [review] 1344834.patch Review of attachment 8863959 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Avikalpa, this looks much better now! Just requires a few more tweaks: be mindful of the whitespaces when you split strings :) It looks like this patch doesn't apply to the latest mozilla-central and requires a new rebase :( Would you mind attaching the rebased and fixed patch for, hopefully, one last pass? As for the implicit string joining, you're correct, my mistake. ::: toolkit/components/telemetry/gen-histogram-data.py @@ +98,5 @@ > n_buckets = histogram.n_buckets() > static_assert(output, "%s < %s" % (low, high), "low >= high for %s" % name) > static_assert(output, "%s > 2" % n_buckets, "Not enough values for %s" % name) > static_assert(output, "%s >= 1" % low, "Incorrect low value for %s" % name) > + static_assert(output, "%s > %s" % (high, n_buckets), "high must be > number of buckets for %s;" nit: there was a space right after %s; , let's restore it :) ::: toolkit/components/telemetry/gen-histogram-enum.py @@ +86,5 @@ > print(" HistogramCount,", file=output) > if seen_use_counters: > + print(" HistogramUseCounterCount = HistogramLastUseCounter -" > + " HistogramFirstUseCounter + 1", > + file=output) nit: can't file=output live on the previous line with the rest of the string? @@ +107,5 @@ > print("template<> struct IsCategoricalLabelEnum<%s> : TrueType {};" % name, file=output) > > print("\ntemplate<class T> struct CategoricalLabelId {};", file=output) > for name, _, id in enums: > + print("template<> struct CategoricalLabelId<%s> :" Let's restore the space after "... CategoricalLabelId<%s> :". This should be "... CategoricalLabelId<%s> : " ::: toolkit/components/telemetry/histogram_tools.py @@ +333,5 @@ > for field in ['alert_emails', 'bug_numbers']: > if field not in definition and name not in whitelists[field]: > raise ParserError('New histogram "%s" must have a "%s" field.' % (name, field)) > if field in definition and name in whitelists[field]: > + msg = 'Histogram "%s" should be removed from the whitelist for "%s" in' \ nit: don't forget about the whitespace after '... "%s" in'. It should be '... "%s" in '.

Comment on attachment 8864129 [details] [diff] [review] 1344834.patch Review of attachment 8864129 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb1d97eeede8d5a1691855fbc5fee1812a92aa0 Bug 1344834 - Enable flake8 rule E501: 'line too long (113 > 99 characters)'. r=Dexter

Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb1d97eeede Enable flake8 rule E501: 'line too long (113 > 99 characters)'. r=Dexter