[chirp_devel] Fwd: [CHIRP - Feature #159] (In Progress) Get pylint happy with chirp
I'd like to get a feeling from others about how valuable they think it is to have a bunch of change to get pylint happy. Personally, I abandoned this work a while ago being ever more frustrated with pylint in a lot of areas. Having used it (or specifically not used it) in other large projects since, I think it's value is extremely limited. For example, it will likely be hopelessly confused by bitwise and any code that uses it. I've seen it employed most usefully in a situation where it is simply used against a specific patch to try to gauge whether it "makes things worse or not".
Unless we pick things we care about specifically for a whitelist of issues, I'd rather not see another hundred or so patches to try to trivially appease pylint.
Thoughts?
--Dan
-------- Forwarded Message -------- Subject: [CHIRP - Feature #159] (In Progress) Get pylint happy with chirp Date: Sun, 8 Mar 2015 03:46:55 -0700 From: donotreply@danplanet.com
Issue #159 has been updated by Zach Welch.
* Tracker changed from Bug to Feature * Status changed from New to In Progress * Assignee changed from Dan Smith to Zach Welch * Target version set to 0.5.0 * % Done changed from 20 to 0 * Chirp Version changed from 0.3.0 to daily
I have a couple of patches in the works to add pylint to the cpep8 script (see #2355 http://chirp.danplanet.com/issues/2355). This allows the source code to be automatically scanned as part of run_all_tests.sh. It includes a new blacklist to exclude files that are not yet compliant.
Unfortunately, not a single file was pylint compliant when I started looking at this task. I was able to get a couple of files fixed up without too much pain, but most of the modules have brutally long lists of issues (far beyond what was flagged/fixed by pep8).
On the bright side, pylint is clearly superior to pep8 in terms of completeness. For example, it already helped me find a latent error in the new logger module. I think that we will be better off with this kind of checking in place.
That said, there are points of contention between the pep8 tool and pylint, so I have disabled a couple of checks that pep8 already says we pass (e.g. line continuation alignment, multiple spaces after commas). There are likely additional checks that need to be disabled.
------------------------------------------------------------------------
Feature #159: Get pylint happy with chirp http://chirp.danplanet.com/issues/159#change-6577
* Author: Dan Smith * Status: In Progress * Priority: Normal * Assignee: Zach Welch * Category: * Target version: 0.5.0 * Chirp Version: daily * Model affected: (All models)
------------------------------------------------------------------------
You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: http://chirp.danplanet.com/my/account
On 03/08/2015 07:53 AM, Dan Smith wrote:
I'd like to get a feeling from others about how valuable they think it is to have a bunch of change to get pylint happy. Personally, I abandoned this work a while ago being ever more frustrated with pylint in a lot of areas. Having used it (or specifically not used it) in other large projects since, I think it's value is extremely limited. For example, it will likely be hopelessly confused by bitwise and any code that uses it. I've seen it employed most usefully in a situation where it is simply used against a specific patch to try to gauge whether it "makes things worse or not".
FWIW, I have patches to de-lint bitwise_grammar.py and bitwise.py, and I think the changes are uniformly beneficial to the code. So far, pylint has had no troubles with bitwise code that weren't rooted in other legitimate issues. The most widespread problem has been the lack of docstrings. Maybe it has matured since you last used it?
Unless we pick things we care about specifically for a whitelist of issues, I'd rather not see another hundred or so patches to try to trivially appease pylint.
I have already eliminated a half-dozen of its checks, because it's just too pedantic (or redundant/conflicting with pep8). I am continuing to tweak the pylintrc file, because I heartily agree that it's far too much to expect 100% conformance to its stock configuration. Nevertheless, pylint's complaints seem less trivial than pep8's.
It gets a solid endorsement from me, because it has already flagged some real errors in the code that need to be fixed. Indeed, I was originally skeptical that it would be worthwhile, since we already comply with pep8; however, these results made me think that it can be tuned to yield significant ongoing value for this project.
At this point, I have almost a dozen patches in my tree, and I think that they add enough value (e.g. docstrings) to justify their inclusion (independent of the fact that they yield pylint compliance). I am working to clean up my stack and post what I have for consideration. As such, please reserve judgment until you see that series hit the list.
FWIW, I have patches to de-lint bitwise_grammar.py and bitwise.py, and I think the changes are uniformly beneficial to the code.
It's not the bitwise code itself, but the code that uses it that is likely most probematic.
The most widespread problem has been the lack of docstrings. Maybe it has matured since you last used it?
I use it almost every day, so, no. There are aspects of python that cannot be statically analyzed -- it's part of the nature of the thing. For example:
class Test(object): def __init__(self, value): setattr(self, 'test_attribute', value)
t = Test() print t.test_attribute
yields this:
E: 6, 6: Instance of 'Test' has no 'test_attribute' member (no-member)
Bitwise is full of that sort of stuff by nature. It's one of the most useful things that pylint can do (identifying things that a compiler would have caught, like using non-existent identifiers), yet it's going to fail miserably on any code that uses bitwise (read: almost all of chirp).
It gets a solid endorsement from me, because it has already flagged some real errors in the code that need to be fixed. Indeed, I was originally skeptical that it would be worthwhile, since we already comply with pep8; however, these results made me think that it can be tuned to yield significant ongoing value for this project.
I'd rather you just write unit tests for things like your logger module and catch the issues that way. Further, like pep8, pylint is a moving target and keeping the config file up to date such that it continues to run on newer versions is actual work, and not work I personally want to do.
--Dan
On 03/08/2015 07:28 PM, Dan Smith wrote: ....
Bitwise is full of that sort of stuff by nature. It's one of the most useful things that pylint can do (identifying things that a compiler would have caught, like using non-existent identifiers), yet it's going to fail miserably on any code that uses bitwise (read: almost all of chirp).
I just had pylint scan the drivers directory, and there is not as much of this as you seem to suggest. At worst, we can use an exceptions file for pylint, as we have for pep8. If there isn't a better solution, those errors can be turned off in those files. In the rest of the files, those are good errors to catch. Moreover, it catches dozens of other classes of issues, so that one possible failing should not disqualify its potential to do good.
It gets a solid endorsement from me, because it has already flagged some real errors in the code that need to be fixed. Indeed, I was originally skeptical that it would be worthwhile, since we already comply with pep8; however, these results made me think that it can be tuned to yield significant ongoing value for this project.
I'd rather you just write unit tests for things like your logger module and catch the issues that way. Further, like pep8, pylint is a moving target and keeping the config file up to date such that it continues to run on newer versions is actual work, and not work I personally want to do.
After looking at the unit testing frameworks and considering the logger module's functionality, I would rather focus my efforts on pylint integration and clean up. Even if I knew where start with writing unit tests for the logger module, that would only benefit the one module. In contrast, pylint integration could benefit the entire code base.
We're using the virtualenv, so moving targets will only be an issue when we choose to move them, right?
Speaking of which, the bigger obstacle (and perhaps a blocker) is getting pygtk and friends installed in a virtualenv, as pylint can't find them if they're not in there. When I went back to clean up cpep8.sh, I discovered that this is not trivial to accomplish on Linux.
participants (2)
-
Dan Smith -
Zach Welch