[SciPy-Dev] Missing whitespace after commas in pycodestyle check

[Joshua Wilson]

Seems like there are no objections to adding the checks to just the
diffs; I'll set that up in upcoming weeks. Very much looking forward
to eliminating that aspect of code review.

- Josh

On Tue, Jan 14, 2020 at 2:47 AM Ralf Gommers <ralf.gommers at gmail.com> wrote:
>
>
>
> On Tue, Jan 14, 2020 at 12:15 AM Ilhan Polat <ilhanpolat at gmail.com> wrote:
>>
>> I actually got pretty quick at fixing these stuff by the aid of some very mild nonaggressive scripts. Hence if we all agree I can go through all repo and fix everything once and ask the current PR owners to rebase.
>
>
> You mean you want to fix up all PRs, right? Or all code in master? If we implement the diff checks I'm not sure the former is needed, but perhaps it helps. If the latter, I don't think that's a good idea.
>
> Ralf
>
>
>>
>> Sounds like a chore to everyone but I guess biting the bullet once and then turning on all the checks seems the least painful way. Otherwise Ralf's suggestion is also a good idea.
>>
>> On Mon, Jan 13, 2020 at 11:10 PM Ralf Gommers <ralf.gommers at gmail.com> wrote:
>>>
>>> Hey Josh,
>>>
>>>
>>> On Mon, Jan 13, 2020 at 1:44 AM Joshua Wilson <josh.craig.wilson at gmail.com> wrote:
>>>>
>>>> Hey everyone,
>>>>
>>>> During code review, it seems that we care about missing whitespace
>>>> after commas. (I have no data on this, but I suspect it is our most
>>>> common linting nit.) Linting for this is currently disabled. Now on
>>>> the one hand, we generally avoid turning on checks like this because
>>>> of the disruption to open PRs and what they do to git history. On the
>>>> other hand, I don't think enforcing this check by hand works.
>>>> Reminding an author to add spaces after commas can easily add several
>>>> rounds of extra review (some places almost invariably get missed on
>>>> the first try, and then the problem tends to sneak back in in later
>>>> commits), and I usually just give up after a while. So I don't think
>>>> we are heading in the direction of "in 5 years all the bad cases will
>>>> be gone and we can turn on the check without disruption".
>>>
>>>
>>> I agree, the current workflow is not optimal.
>>>
>>>>
>>>> So I would kind of like to push us to either:
>>>>
>>>> 1. Turn the lint check on. We can do this module-by-module or even
>>>> file-by-file if necessary to minimize disruption.
>>>> 2. Declare that this is not something we care about in code review.
>>>
>>>
>>> Another option would be to run pycodestyle/pyflakes with all checks we want enabled, but only on the diff in the PR. This is what PyTorch does for example. The nice thing is you can test what you want without having to deal with code churn before enabling a check. The downside is that it's a bit convoluted to reproduce the "run on the diff to master" command locally (but we could add a script to streamline that probably).
>>>
>>> Cheers,
>>> Ralf
>>>
>>> _______________________________________________
>>> SciPy-Dev mailing list
>>> SciPy-Dev at python.org
>>> https://mail.python.org/mailman/listinfo/scipy-dev
>>
>> _______________________________________________
>> SciPy-Dev mailing list
>> SciPy-Dev at python.org
>> https://mail.python.org/mailman/listinfo/scipy-dev
>
> _______________________________________________
> SciPy-Dev mailing list
> SciPy-Dev at python.org
> https://mail.python.org/mailman/listinfo/scipy-dev