Fix warnings found by pylint #973

New issue

Closed

opened 2010-02-26 05:34:04 +00:00 by daira · 18 comments

daira commented

2010-02-26 05:34:04 +00:00

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.

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.

daira added the

labels 2010-02-26 05:34:04 +00:00

daira added this to the 1.7.0 milestone 2010-02-26 05:34:04 +00:00

daira commented

2010-02-26 05:36:40 +00:00

Author

Attachment pylint.html (715098 bytes) added

output of pylint --rcfile=tahoe.pylintrc allmydata

**Attachment** pylint.html (715098 bytes) added output of pylint --rcfile=tahoe.pylintrc allmydata

pylint.html

698 KiB

daira commented

2010-02-26 05:37:56 +00:00

Author

Attachment tahoe.pylintrc (9419 bytes) added

pylint configuration file to reduce obvious false-positives

**Attachment** tahoe.pylintrc (9419 bytes) added pylint configuration file to reduce obvious false-positives

tahoe.pylintrc

9.2 KiB

daira commented

2010-02-26 05:38:44 +00:00

Author

Attachment correct-indentation-darcspatch.txt (60779 bytes) added

Correct harmless indentation errors found by pylint

**Attachment** correct-indentation-darcspatch.txt (60779 bytes) added Correct harmless indentation errors found by pylint

correct-indentation-darcspatch.txt

59 KiB

daira commented

2010-02-26 07:36:51 +00:00

Author

Attachment change-imports-to-absolute-darcspatch.txt (69877 bytes) added

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

**Attachment** change-imports-to-absolute-darcspatch.txt (69877 bytes) added Change relative imports to absolute. Relative imports are ambiguous and [will not be supported with the current syntax in Python 2.7](http://www.python.org/dev/peps/pep-0328/).

change-imports-to-absolute-darcspatch.txt

68 KiB

daira commented

2010-02-26 08:04:51 +00:00

Author

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

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

writefaruq commented

2010-05-07 20:52:47 +00:00

Owner

Attachment FORMAT-checked-output.html (36847 bytes) added

**Attachment** FORMAT-checked-output.html (36847 bytes) added

FORMAT-checked-output.html

36 KiB

writefaruq commented

2010-05-07 20:53:28 +00:00

Owner

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

writefaruq commented

2010-05-07 20:55:45 +00:00

Owner

Attachment FORMAT-checked-output.txt (16025 bytes) added

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

**Attachment** FORMAT-checked-output.txt (16025 bytes) added After applying "correct-indentation-darcspatch.txt" patch to current source and then running pylint FORMAT checker

FORMAT-checked-output.txt

16 KiB

zooko commented

2010-05-08 05:01:21 +00:00

Attachment scrot.png (282579 bytes) added

**Attachment** scrot.png (282579 bytes) added

scrot.png

276 KiB

zooko commented

2010-05-08 05:06:58 +00:00

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]source:src/allmydata/check_results.py@4253#L21.

    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!

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](wiki/CodingStandards) 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]source:src/allmydata/check_results.py@4253#L21. ``` 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 commented

2010-05-08 20:52:14 +00:00

Owner

Attachment FORMAT-checked-output.2.txt (3590 bytes) added

Ignoring C0301, C0321

**Attachment** FORMAT-checked-output.2.txt (3590 bytes) added Ignoring C0301, C0321

FORMAT-checked-output.2.txt

3.5 KiB

writefaruq commented

2010-05-08 21:44:50 +00:00

Owner

Attachment IMPORTS-checked-output-2.txt (47593 bytes) added

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

**Attachment** IMPORTS-checked-output-2.txt (47593 bytes) added Reports import failed: F0401(128 times), deprecated module: W0402(2), Reimport:W0404(3), self import:W0406(1)

IMPORTS-checked-output-2.txt

46 KiB

writefaruq commented

2010-05-08 21:49:42 +00:00

Owner

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

zooko commented

2010-05-08 22:33:20 +00:00

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")'

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")' ```

daira commented

2010-05-08 22:47:55 +00:00

Author

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]source:src/allmydata/check_results.py@4253#L21.
    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.

Replying to [zooko](/tahoe-lafs/trac/issues/973#issuecomment-376791): > 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](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/973/tahoe.pylintrc#L275)), 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]source:src/allmydata/check_results.py@4253#L21. > ``` > 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 commented

2010-05-09 21:36:09 +00:00

Owner

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. :)

Replying to [zooko](/tahoe-lafs/trac/issues/973#issuecomment-376793): > 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 commented

2010-05-14 19:33:16 +00:00

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

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