On 03/10/2015 08:45 PM, Tom Hayward wrote:
On Mon, Mar 9, 2015 at 7:40 AM, Zach Welch zach@mandolincreekfarm.com wrote:
I agree that many of the module-level docstrings that I added need improvement, but their presence serves as a reminder to add more information down the road.
A much better reminder that a docstring needs improvement is a warning from pylint. It looks like what you've done is provided a low-quality placeholder just to shut up pylint. This kind of thing defeats the purpose of pylint. I'm failing to see the value in this change.
That's a valid observation, but it drives at the more fundamental reason that I was working on this issue.
At the very start, I added pylint to the automatic checking script, thinking that it would be good to have it run as part of a continuous integration script. Like the pep8 check, this will elevate the quality of all future patches. Clearly, that requires all of the pylint checks to pass for a given file, or the continuous integration check will fail.
Without fixing every issue, it can't be used continuously. If it is not part of a standard and automated review/commit process, then it will not be used universally by developers, so it might as well not be used at all.
Without enabling all of the checks that we want enforced, we cannot expect new code to achieve that standard, as it will not be caught by the integration scripts. Sure, not every check needs to be turned on, so the broader question here is what standards do the CHIRP developers care to enforce. Obviously, I think docstrings should be mandatory, thus I added a placeholder.
Right now, it seems clear that no one has been running pylint at all, so its purpose -- to be used -- has already been defeated. As such, I think that getting the tree to a point that it runs cleanly -- with all of the checks that should be enabled -- does have clear and present value. In the context of improving the continuous integration process, a placeholder is tangibly better than nothing at all. Without that ulterior motive, I would agree that the change has no value.
Now, take another step back for even more perspective. I would be happy to write suitable documentation to replace the placeholder, but I think it would be unreasonable to block these patches until I do that. Instead, we should embrace iterative and incremental development. Specifically, I need time to grok things sufficiently in order to write good documentation. In the meantime, I will do what I can to move things forward, and I think automating pylint moves us forward.