[chirp_devel] [PATCH 06/11] Fix pylint issues in __init__.py files (#159)

[Dan Smith]

> Like the pep8 check, this will elevate the quality of all future
> patches.

I disagree. The pep8 checks (especially the ones we have enabled)
enforce basic consistency and basic mechanics. Requiring every tiny and
obvious utility function to have a docstring is not equivalent to
quality.  Writing functions that are small in scope and clearly-readable
is, IMHO. Except in a public interface, the latter obviates the need for
the former in almost ever case.

> Clearly, that requires all of the pylint checks to pass for a given
> file, or the continuous integration check will fail.

Maybe this will help: I really have no intention of making passing
pylint (as configured) a requirement for all patches.

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

I care far more about people that write drivers, add features to
drivers, or fix bugs in drivers. A lot of those contributions come from
people that aren't going to tolerate a high level of obsession over
things that appear trivial to them. Since I'm not going to bring myself
to reject their very useful and functional patch because of such a
missing thing, if I enforce it before commit, then I have to do the work
to fix them up each time.

Further, I don't want to elevate the bar so high that it appears to not
be worth even trying.

I appreciate what you're trying to do, and in an ideal world, we'd
completely pass all static analysis everywhere all the time. I just
don't think it's appropriate for a project like this.

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

Since I don't think that "useful" documentation is needed on each of the
placeholder-documented things, I don't think I'm going to change my mind
about the need to enforce them until I see them.

Note that I'm not saying "show me them and I'll be convinced." I'm
saying I agree that these have little value. If I thought the
end goal was extremely worthwhile, then the iterative approach to
getting us there would make sense.

> Instead, we should embrace iterative and incremental development.

Can we add pragmatic to the above list? Start with a list of pylint
rules that we can all agree on and move that forward. The changes like
the ones that avoid conflicts with reserved words are reasonable. Let's
make those fixes and get pylint gating on those. Then we can
discuss/argue about raising the bar on specific details and consider
each on its own merits.

--Dan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
Url : http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20150311/1fd6db89/attachment-0001.bin