[Buildbot-devel] Running Pylint on Buildbot's source code

[Dustin J. Mitchell]

On Thu, Jul 11, 2013 at 1:53 AM, Elmir Jagudin <elmir.jagudin at axis.com> wrote:
> On 07/05/2013 04:31 AM, Dustin J. Mitchell wrote:
>
> :C0112 (empty-docstring): *Empty docstring*
>   Used when a module, function, class or method has an empty docstring (it
> would
>   be too easy ;). This message belongs to the basic checker.
>
> Is there a point with empty docstrings?
>
> Otherwise I think we should get rid of them, so it will be clear that it is
> not documented.

Good point.

> :C0102 (blacklisted-name): *Black listed name "%s"*
>   Used when the name is listed in the black list (unauthorized names).
> This message belongs to the basic checker.
>
> This is a configurable list of black listed names. The default is
> 'foo,bar,baz,toto,tutu,tata'.
>
> I think this should be fixed, by using more descriptive names.

It looks like they're all in tests, where IMHO foo is an acceptable
placeholder meaning "don't care".

> :E0102 (function-redefined): *%s already defined line %s*
>   Used when a function / class / method is redefined. This message
> belongs to the basic checker.
> *** pyflakes catches these, so I suspect anything pylint has found is
> a false alarm?
>
>
> Well, technically speaking these are not false alarms, pylint finds these
> two redefinitions:
>
> [1]:
> https://github.com/buildbot/buildbot/blob/9cd7f9a097f16bc8e0e1f8dbcf52b1946b5c1756/master/buildbot/status/web/change_hook.py#L81
> [2]:
> https://github.com/buildbot/buildbot/blob/9cd7f9a097f16bc8e0e1f8dbcf52b1946b5c1756/master/buildbot/db/pool.py#L98
>
> Both of these are actual redefinitions of local variables.
>
> IMHO [1] should be fixed by renaming exception variable on line 63.
>
> Not sure about [2], the code can be changed to:
>
>         log_msg = log.msg
>         if verbose:
>             def _log_msg(m):
>                 print m
>             log_msg = _log_msg
>
> or by just disabling E0102 there.
>
> Anyway I think this check is good to have. As an extra safety net, in case
> pyflakes misses something.

Agreed.  I think your fix for log_msg is good.

> :E0211 (no-method-argument): *Method has no argument*
>   Used when a method which should have the bound instance as first argument
> has
>   no argument defined. This message belongs to the classes checker.
> *** this may be from the interfaces?
>
> Yes, this is only found in interface definitions. This should be disabled
> locally for interfaces, as it is done in other places.
>
> :E0213 (no-self-argument): *Method should have "self" as first argument*
>   Used when a method has an attribute different the "self" as first
> argument.
>   This is considered as an error since this is a so common convention that
> you
>   shouldn't break it! This message belongs to the classes checker.
> *** this may be from the interfaces?
>
> Most of this is interfaces, except one place:
>
> https://github.com/buildbot/buildbot/blob/master/master/buildbot/test/unit/test_steps_source_svn.py#L1693

Huh, weird.

> IMHO we should handle E0213 the same way as E2011.

Agreed.

Dustin