Fix warnings found by pylint

[davidsarah]

I tried running pylint on the Tahoe source. It produced a lot of false alarms, but some things that are worth fixing. The HTML output, and an initial patch to fix some indentation errors are attached.

[davidsarah]

output of pylint --rcfile=tahoe.pylintrc allmydata

[davidsarah]

pylint configuration file to reduce obvious false-positives

[davidsarah]

Correct harmless indentation errors found by pylint

[davidsarah]

Change relative imports to absolute. Relative imports are ambiguous and ​will not be supported with the current syntax in Python 2.7.

[davidsarah]

Well, [apparently ​http://old.nabble.com/Absolute-imports-in-Python-2.x--td27419560.html] removal of support for relative imports didn't happen, but they're a bad idea anyway.

[writefaruq]

After applying "correct-indentation-darcspatch.txt" patch to current source and then running pylint FORMAT checker I got a few warnings.

[writefaruq]

After applying "correct-indentation-darcspatch.txt" patch to current source and then running pylint FORMAT checker

[zooko]

Dear Faruq:

Blah, I just scanned through FORMAT-checked-output.txt and I'm not impressed with pylint. All the notes I saw were "line too long" and "multiple statements on a single line". Most but not all were pointing at code that I wrote. My assumption is that the lines in question are better written the way they are and we should stop running pylint and ban it from our project. Note that our coding standards explicitly reject the convention of limiting lines to 72- or 79- chars.

Okay, okay, I'll look closer.

Here's the first "line too long" that I looked at: check_results.py line 21​.

    def set_healthy(self, healthy):
        self.healthy = bool(healthy)
        if self.healthy:
            assert (not hasattr(self, 'recoverable')) or self.recoverable, hasattr(self, 'recoverable') and self.recoverable
            self.recoverable = True
            self.summary = "healthy"
        else:
            self.summary = "not healthy"

Attached as "scrot.png" is a screenshot of what this method looks like in my emacs side by side with what it looks like after being line-wrapped. I prefer the unwrapped version! Note that my emacs automatically line-wraps the line that extends past the width of the frame but (unfortunately) does not indent the continuation. I prefer this style: let carriage returns signify logical breaks, i.e. the ends of statements, and let your tools (trac, emacs, vi, screen, your terminal emulator) do formatting for visual purposes. For example, this makes "grep" tell me what I want to know:

HACK Wonwin-McBrootles-Computer:~/playground/tahoe-lafs/trunk/pristine$ grep assert src/allmydata/check_results.py
        assert IURI.providedBy(uri), uri
            assert (not hasattr(self, 'recoverable')) or self.recoverable, hasattr(self, 'recoverable') and self.recoverable
            assert (not hasattr(self, 'healthy')) or not self.healthy
        assert isinstance(summary, str) # should be a single string
        assert not isinstance(report, str) # should be list of strings
        assert isinstance(path, (list, tuple))
        assert isinstance(path, (list, tuple))

Where with the line-wrapping grep is now giving me an incomplete answer about that line:

HACK Wonwin-McBrootles-Computer:~/playground/tahoe-lafs/trunk/pristine$ grep assert src/allmydata/check_results-pylinted.py
        assert IURI.providedBy(uri), uri
            assert (not hasattr(self, 'recoverable')) or
            assert (not hasattr(self, 'healthy')) or not self.healthy
        assert isinstance(summary, str) # should be a single string
        assert not isinstance(report, str) # should be list of strings
        assert isinstance(path, (list, tuple))
        assert isinstance(path, (list, tuple))

Okay, so let's ignore, at least for now, all instances of C0301 "line too long" and C0321 "More than one statement on a single line". In fact, let's add those two to the list of "disable-msg" in ​http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/973/tahoe.pylintrc and let's add that pylintrc file into our source repo (somewhere) for other people to use).

And then, please read through David-Sarah's patches correct-indentation-darcspatch.txt and change-imports-to-absolute-darcspatch.txt and see if there are any mistakes in it. :-)

Thanks!

[writefaruq]

Ignoring C0301, C0321

[writefaruq]

Reports import failed: F0401(128 times), deprecated module: W0402(2), Reimport:W0404(3), self import:W0406(1)

[writefaruq]

Need to see why so many import fails. Is this a false alarm or feature of pylint ? I'm happy with all else

[zooko]

Well, those are imports of external libraries, and I guess maybe you don't have them installed. For example, the first one is

F0401: 26: Unable to import 'nevow'

Do you have the "nevow" package installed? Here is a good way to tell:

python -c 'import pkg_resources;print pkg_resources.require("nevow")'

[davidsarah]

Replying to zooko:

Blah, I just scanned through FORMAT-checked-output.txt and I'm not impressed with pylint.

Well, that's just the formatting checker, which is one of the least interesting. The maximum line length is configured at 120 (​here), and personally I think that is too long in most cases.

Here's the first "line too long" that I looked at: check_results.py line 21​.

    def set_healthy(self, healthy):
        self.healthy = bool(healthy)
        if self.healthy:
            assert (not hasattr(self, 'recoverable')) or self.recoverable, hasattr(self, 'recoverable') and self.recoverable
            self.recoverable = True
            self.summary = "healthy"
        else:
            self.summary = "not healthy"

Attached as "scrot.png" is a screenshot of what this method looks like in my emacs side by side with what it looks like after being line-wrapped. I prefer the unwrapped version!

That's because you wrapped it at 65 characters, if I'm counting correctly. It looks much better (and would not have any problems with grepping for subexpressions) when wrapped at the comma, which is still well below 120 characters.

I'm not suggesting that we should spend a lot (or any) effort on fixing minor formatting issues like this, though.

[writefaruq]

Replying to zooko:

Well, those are imports of external libraries, and I guess maybe you don't have them installed. For example, the first one is

F0401: 26: Unable to import 'nevow'

Do you have the "nevow" package installed? Here is a good way to tell:

python -c 'import pkg_resources;print pkg_resources.require("nevow")'

Yes, that's right. I installed a few of those libraries and these messages vanished. :)

[zooko]

faruq: if you could configure the tahoe.pylintrc to exclude C0301, C0321 and then put the .pylintrc file into misc and run darcs add misc/tahoe.pylintrc and darcs record then we can get that configuration file into our source tree.

[davidsarah]

Applied in e76092e16c640198 and a80f19a084c1fcb1.