Closed Bug 1124530 Opened 10 years ago Closed 9 years ago

Automatic Python Static Analysis

Categories

(MozReview Graveyard :: General, defect, P4)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1196263

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

See summary.
Attached file MozReview Request: bz://1124530/gps (obsolete) —
/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r a261928f11bd4d3ab94ca9e2aa78bf8b9f6fa512
Attachment #8552824 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/2835/#review2081

This is looking really good! I'm really excited to see this actually running :D

::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Can we include a note about where this was taken from and that notifying upstream of changes would be appreciated. It'd be great if improvements (such as speeding up the line translation algorithms etc.) could eventually filter back to Review Bot if we make them.

To clarify, I don't think we should let compatability with Review Bot slow us down, but if I can harvest any changes RBot could use the love.

::: pylib/mozreviewbots/requirements.txt
(Diff revision 1)
> +flake8==2.3.0
> +mccabe==0.3
> +Mercurial==3.2.4
> +pep8==1.5.7

Are we planning to have all the tools we write live in this package (flake8 in this case)? or just the easily installed python ones?

It might be nice for people to write their own packages taking advantage of the consumer / review abstractions without having to install a bunch of packages they don't care about.

::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 1)
> +        self.exchange = Exchange(exchange, type='topic', durable=True)

The exchange used by MozReview (at least on the producer side) is hardcoded. Maybe we should default the exchange to the standard MozReview one?

I wonder if it'd be worth allowing configuration of the exchange on the producers side?

::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 1)
> +        with self.conn.Consumer([self.queue], callbacks=[onmessage], auto_declare=True):
> +            try:
> +                self.conn.drain_events(timeout=timeout)
> +            except socket.timeout:
> +                pass

This will grab all of the messages sitting on the queue when called and then return, correct?

Wouldn't it be better to only consume 1 message at a time, and then only ack them once the operation is complete? That way if this worker goes down any messages which weren't completely processed will still exist in the queue.

(All that being said, having this function which yeilds out the messages is a pretty nice interface, so maybe offering both options would be the right way to go?)

::: pylib/mozreviewbots/setup.py
(Diff revision 1)
> +    author='Mozilla',
> +    author_email='dev-version-control@lists.mozilla.org',

Ah, good call, we should switch all the other packages to use the same thing.

Filed Bug 1124642

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 1)
> +from .consumer import PulseConsumer
> +from .review import Review
> +from .util import (
> +    ensure_hg_repo_exists,
> +    hg_commit_changes,
> +)

These through me off, haven't really used / seen them and I actually commented on one of mconley's review requsts to remove it.

I've read the PEP through now though so I think I'm comfortable with it. Has this gained wide adoption in the Python world now?

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +import uuid
> +
> +HG = None

I'd prefer an extra blank line here (PEP8 appears to be ambiguous on the matter)

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +    sha1 = hashlib.sha1(url).hexdigest()
> +    repo_path = os.path.join(root, sha1)

ooo, good idea :)

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +        _run_hg(['clone', url, repo_path])

So currently this will clone the entire review repo, with all the heads. That probably isn't a problem with the amount of traffic we have, but would be later.

But I'm assuming you're banking on the review repo being the S3 saved bundles thing, so this would just clone the base? I'll leave this comment here on the off chance this isn't what you're thinking so you can correct me.

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +        if pull:

I think this argument is a little too ambiguous. Can we call it `pull_rev` or `ensure_rev` or something?

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +            env=env, cwd='/')

`cwd='/'`, we always want to have the working directory be the filesystem root - Is this you forcing people to *always* specify by path which repository they're working with?

Please comment this reason.

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +    """Obtain information about what changed in a Mercurial commit."""

Idea that just poppped into my head: you could totally write a commit message linter that checks a number of things.

Including a warning "This is a large change and your commit message only contained a summary line..." if the size of the change is over some arbitrary threshold (or at all) - I can see that really annoying some people though.

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 1)
> +    return adds, dels, mods, None, diff

While it's possible to guess, the only place there is documentation for what the `None` represents is at a call site (due to variable naming). Can we add some sort of comment, or in the docstring what the return value represents.

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 1)
> +    # process boundaies. Specify jobs=0 to get results.

"boundaies" -> "boundaries"

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 1)
> +    try:
> +        os.chdir(repo_path)
> +        return style.check_files(relevant)
> +    finally:
> +        os.chdir(oldcwd)

I can't find anywhere we'd actually catch the exception this can throw, I believe it will actually make it all the way up and crash.

As just discussed in person we need to figure out a way to handle errors, etc. I'll just leave this as a reminder.

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 1)
> +        review_hg_review_request(api_root, review_request_id, repo_path, node)

As discussed in person you need to pass in rrid here instead of review_request_id, you've probably already fixed it but I'll put this here anyways.

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 1)
> +def review_hg_review_request(api_root, review_request_id, repo_path, node):

Once I start passing the diff revision number down with the pulse messages we'll have to pass it in here and give it to the Review object to ensure the comments are on the correct revision.

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 1)
> +            logging.info('received a message from Pulse')

Logging is great, might be worth sprinkling some debug logging around with a configurable log level?

::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 1)
> +
> +import json
> +import os

We'll probably want to import unicode_literals as well

::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 1)
> +        # TODO diff revision should be passed in.

Ah okay, you have the TODO here, nice.

::: pylib/mozreviewbots/tests/hghave
(Diff revision 1)
> +#!/usr/bin/env python
> +
> +import os
> +
> +HERE = os.path.abspath(os.path.dirname(__file__))
> +REPO_ROOT = os.path.join(HERE, '..', '..', '..')
> +execfile(os.path.join(REPO_ROOT, 'testing', 'hghave.py'))

I don't see us using this yet, are we just preparing for the future?
Depends on: 1124845
https://reviewboard.mozilla.org/r/2835/#review2121

> Are we planning to have all the tools we write live in this package (flake8 in this case)? or just the easily installed python ones?
> 
> It might be nice for people to write their own packages taking advantage of the consumer / review abstractions without having to install a bunch of packages they don't care about.

I don't know yet. I plan on using this requirements.txt to install the Python bot.

For sanity reasons, I think it makes sense for every consumer to share environments, if possible.

> I don't see us using this yet, are we just preparing for the future?

This is magic required by the test harness. Basically, we need this here so `#require docker` in `.t` files works.

> This will grab all of the messages sitting on the queue when called and then return, correct?
> 
> Wouldn't it be better to only consume 1 message at a time, and then only ack them once the operation is complete? That way if this worker goes down any messages which weren't completely processed will still exist in the queue.
> 
> (All that being said, having this function which yeilds out the messages is a pretty nice interface, so maybe offering both options would be the right way to go?)

We definitely need something better here. I'll put something together for v2.
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r 75cc086e4fa6570527512b8a0fa6f0b328555ffe
https://reviewboard.mozilla.org/r/2835/#review2123

Always look on the bright side of life.
I analyzed your Python changes and found 21 errors.

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py

::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 2)
> +            queue, ssl=False):

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 2)
> +                userid=userid, password=password, ssl=ssl)

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 2)
> +            routing_key='mozreview.commits.published',

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 2)
> +            exclusive=False, auto_delete=False)

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 2)
> +        def onmessage(body, message):

E301: expected 1 blank line, found 0
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: pylib/mozreviewbots/mozreviewbots/consumer.py
(Diff revision 2)
> +        with self.conn.Consumer([self.queue], callbacks=[onmessage], auto_declare=True):

E501: line too long (88 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +    review_request = api_root.get_review_request(review_request_id=review_request_id)

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +            'I analyzed your Python changes and found %d errors.' % res.total_errors,

E501: line too long (85 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +                'with this patch!',

E131: continuation line unaligned for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +            pulse_password, pulse_exchange, pulse_queue, ssl=pulse_ssl)

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +                repo_path, commit['rev'])

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +    import sys

F811: redefinition of unused 'sys' from line 10

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +            help='Path to config file to load')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +            help='How long to wait for messages per iteration')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +            help='Whether to run forever')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 2)
> +        forever=args.forever))

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 2)
> +                       "%d of %d.") % (max_comments, len(self.comments))

F821: undefined name 'max_comments'

::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 2)
> +            del self.comments[max_comments:]

F821: undefined name 'max_comments'

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 2)
> +    _run_hg(['-R', repo_path, '--config', 'extensions.purge=', 'purge', '--all'])

E501: line too long (81 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.


::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 2)
> +        'strip', '--no-backup', '-r', 'not public()'])

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revision 2)
> +            env=env, cwd='/')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r 883b602f2278a4df3588100d17f05d36c981bbd3
https://reviewboard.mozilla.org/r/2835/#review2125

Always look on the bright side of life.

I analyzed your Python changes and found 4 errors.

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 3)
> +            review_request_id=review_request_id)

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 3)
> +            f.comment(comment, line, num_lines=num_lines, issue=True)

E303: too many blank lines (2)
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 3)
> +            pulse_host, pulse_port, pulse_userid, pulse_password,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 3)
> +    sys.exit(run_pylint_deamon(config, timeout=args.timeout,
> +        forever=args.forever))

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)
Should the newlines in the outlines show up as \n or as actual newlines?
We're still working it out. There's a reason the patch hasn't landed yet. Sorry if our beta testing a live audience causes any trouble.
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r 4147d37b40943586a428103cd2e4a91d25ff76ed
https://reviewboard.mozilla.org/r/2835/#review2153

Always look on the bright side of life.

I analyzed your Python changes and found 4 errors.

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 4)
> +    review_request = api_root.get_review_request(
> +            review_request_id=review_request_id)

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 4)
> +            f.comment(comment, line, num_lines=num_lines, issue=True)

E303: too many blank lines (2)
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 4)
> +    consumer = PulseConsumer(
> +            pulse_host, pulse_port, pulse_userid, pulse_password,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 4)
> +    sys.exit(run_pylint_deamon(config, timeout=args.timeout,
> +        forever=args.forever))

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r afaeb5f9fd8b2e19662cc9ccda3b8cc8944b323c
https://reviewboard.mozilla.org/r/2835/#review2155

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r c1ba67230f17435b64d6ca87e0c927a4fcacb41d
https://reviewboard.mozilla.org/r/2835/#review2167

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r 3423018247c9a670d414637e5d277e411f3364a0
https://reviewboard.mozilla.org/r/2835/#review2201

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py
https://reviewboard.mozilla.org/r/2835/#review2169

::: pylib/mozreviewbots/tests/test-python-noop.t
(Diff revision 6)
> +
> +  $ rbmanage dumpreview $HGPORT1 2

Can we add some comment text here saying we expect 0 reviews to be shown in this dump.

::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 6)
> +        TODO: Change this to check all chunks within a range for a
> +        modification. Currently the pep8 tool will only single line
> +        comment, but future tools might multi-line.
> +        """
> +        line_num_index = 4
> +        if original:
> +            line_num_index = 1
> +
> +        for chunk in self.diff_data.chunks:
> +            for row in chunk.lines:
> +                if row[line_num_index] == line_num:
> +                    return not (chunk.change == 'equal')

We need to actualy fix this TODO becuase our intelligent range comments may move the first line into an 'equal' region blocking it as a comment.

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revisions 1 - 6)
> +            # Extend line continuation errors to multiple lines.
> +            if code in ('E121', 'E122', 'E126', 'E127', 'E128', 'E131'):
> +                line = max(line - 1, 0)
> +                num_lines = 2
> +            # Blank lines before.
> +            elif code in ('E301', 'E302'):
> +                line = max(line - 2, 0)
> +                num_lines = 3

It'd be nice to break ths out into some sort of dispatch table, mapping error codes to the operations, rather than burrying it inside this method.

::: pylib/mozreviewbots/mozreviewbots/util.py
(Diff revisions 1 - 6)
> +    # Execute at / to prevent Mercurial's path traversal logic from kicking in
> +    # and picking up unwanted config files.

Ah, very nice.

::: pylib/mozreviewbots/mozreviewbots/review.py
(Diff revision 7)
> +        if modified and issue:

We should only be asking `if modified` here. We still want to publish the comment even if it doesn't open an issue.

inside of the `if modified` block is where you should do:
```
if issue:
    self.review.ship_it = False
```

So in total:
```
if modified:
    if issue:
        self.review.ship_it = False
    
    self._comment(text, real_line, num_lines, issue)
    return True
```
https://reviewboard.mozilla.org/r/2835/#review2265

> We definitely need something better here. I'll put something together for v2.

So it turns out I was never acking messages after consuming them. This is now fixed. It shouldn't matter how many messages we consume from the queue if we never end up acking them. There are concerns about multiple queue consumers. But let's cross that bridge later.
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)

Pull down this commit:

hg pull review -r 127e37ff189aa0b7ae5a1bb138381d161b3da786
https://reviewboard.mozilla.org/r/2835/#review2267

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)
/r/2977 - mozreviewbots: use proper diffset revision

Pull down these commits:

hg pull review -r 87cc8c7223f69475183b2bf13a77b5915d380537
https://reviewboard.mozilla.org/r/2835/#review2269

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/__init__.py
  pylib/mozreviewbots/mozreviewbots/consumer.py
  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
  pylib/mozreviewbots/mozreviewbots/util.py
  pylib/mozreviewbots/setup.py
https://reviewboard.mozilla.org/r/2977/#review2271

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreviewbots/mozreviewbots/pylint.py
  pylib/mozreviewbots/mozreviewbots/review.py
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

/r/2835 - mozreviewbots: introduce a project for running static analysis (bug 1124530)
/r/2977 - mozreviewbots: use proper diffset revision

Pull down these commits:

hg pull review -r 87cc8c7223f69475183b2bf13a77b5915d380537
https://reviewboard.mozilla.org/r/2835/#review2279

::: pylib/mozreviewbots/mozreviewbots/pylint.py
(Diff revision 8)
> +            random.choice(ERRORS_QUIPS),

I like this.

That being said, as soon as we add more than one quip testing is going to suck. What do you have planned for that?
https://reviewboard.mozilla.org/r/2835/#review2281

> I like this.
> 
> That being said, as soon as we add more than one quip testing is going to suck. What do you have planned for that?

By suck I mean we'll be able to solve it no problem with (glob) etc. ignore me... heh
Comment on attachment 8552824 [details]
MozReview Request: bz://1124530/gps

removing the r? until this is picked back up later.
Attachment #8552824 - Flags: review?(smacleod)
This is still very useful, but we should fix a bunch of papercuts first.
Priority: -- → P3
Depends on: 1125473
We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.
Priority: P3 → P4
Attachment #8552824 - Attachment is obsolete: true
I'm taking this as follow on work to the static analysis bot library I'm working on in Bug 1196263.
Assignee: nobody → dminor
Blocks: 1196263
Status: NEW → ASSIGNED
No longer blocks: 1196263
Depends on: 1196263
Dropping this since we're lowering the priority on static analysis work.
Assignee: dminor → nobody
Status: ASSIGNED → NEW
I'm going to resolve this as a dup of 1196263 as the gps's code has been merged into the library being developed there. Getting this deployed requires changes to our pulse publishing and deserves its own bug.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: