s/Got doesn’t have/Git doesn’t have/
(iPhone autocorrect, sorry)
--a
> On 2 Dec 2020, at 19:01, Andrew Yourtchenko via lists.fd.io
> <ayourtch=gmail....@lists.fd.io> wrote:
>
>
>
>>> On 2 Dec 2020, at 18:21, Paul Vinciguerra <pvi...@vinciconsulting.com>
>>> wrote:
>>>
>>
>>
>>
>>> On Wed, Dec 2, 2020 at 10:57 AM Andrew 👽 Yourtchenko <ayour...@gmail.com>
>>> wrote:
>>>
>>>
>>>>> On 2 Dec 2020, at 16:42, Paul Vinciguerra <pvi...@vinciconsulting.com>
>>>>> wrote:
>>>>>
>>>>
>>>>
>>>>
>>>>> On Wed, Dec 2, 2020 at 5:38 AM Andrew 👽 Yourtchenko <ayour...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>
>>>>> > On 2 Dec 2020, at 10:38, Klement Sekera via lists.fd.io
>>>>> > <ksekera=cisco....@lists.fd.io> wrote:
>>>>> >
>>>>> > I like the idea.
>>>>> >
>>>>> > Regarding maintainer pain:
>>>>> >
>>>>> > Q: are we now stuck forever with what we have because there will always
>>>>> > be somebody facing some difficulties adopting?
>>>>>
>>>>> To me it is always a question of cost/benefit analysis - and admittedly
>>>>> the value of “cost” and “benefit” is always subjective, and not the same
>>>>> in time, so let me expand on why I suggested why suggested:
>>>>>
>>>>> - I can see a benefit in having a target “make test-fixstyle”, similar
>>>>> with C code, introduced sometime *just* before the LTS branch pull, such
>>>>> that we have a chance to be able to sync the changes.
>>>>>
>>>>> - I can see less benefit in introducing that target now, so soon after
>>>>> the LTS release - since that would mean the moment you run a .py file on
>>>>> master through the formatter, as a developer now you will have to
>>>>> manually deal with *two* versions of file - in master and LTS. And
>>>>> porting the diffs that don’t apply due to python formatting is a highly
>>>>> unhappy task.
>>>>
>>>> I don't know if git 2.23 helps you there. If we make the change now
>>>> (figuratively), the issue goes away in theory this time next year as the
>>>> old releases go out of support.
>>>
>>> Right. And until then nearly every change involving test code changes will
>>> have to be backported to 20.09 manually by the feature developers. I would
>>> it’s the net increase of work, with less trivial yet still very sad job of
>>> dealing with reshuffling the patches. PEP501-type annoyance, but with more
>>> lines and higher chances of error.
>>
>> So, this is a different issue to me. Because we don't like chained commits,
>> this is an issue. To do this properly in my mind, that is to make your job
>> easier, there are dependent changes needed. One tuning the build system to
>> support the "black" pep8 output, the second with the changes introducing
>> black and the file changes.
>>
>> If the first change were backported, then post black changes shouldn't cause
>> the stable to barf on it.
>
> Got doesn’t have an idea about dependent changes.
>
>>
>>>
>>>>
>>>>>
>>>>> - Having this done automatically as a precommit hook and removing it from
>>>>> the checkstyle job means:
>>>>>
>>>>> 1) we trust the client to run the formatting themselves. This is a recipe
>>>>> to style inconsistency due to *whatever* unrelated things like merges,
>>>>> etc.
>>>>>
>>>>> 2) forcefully running it just seems like a unsound idea since then we are
>>>>> going to get arbitrary white space changes whenever someone of the
>>>>> formatter devs decides to change the python formatter. I tried formatters
>>>>> for C, golang, rust to name a few. *none* of them produce the identical
>>>>> result from differently white-spaced code, that is functionally
>>>>> identical, even if the formatter is part of the language.
>>>>>
>>>>> In addition this approach shares all the drawbacks of introducing the
>>>>> change at the inopportune moment.
>>>>
>>>> I'm not saying +2 the change today. ;). I'm trying to socialize the idea
>>>> and see what folks think.
>>>
>>> Ah ok :)
>>>
>>>>
>>>>>
>>>>> That’s the reason I suggested to just add the E501 to ignore and move on
>>>>> - on balance it seems like their relieves the maximum amount of real pain
>>>>> without introducing unknown amounts of new one.
>>>> Going to a line length of 88 (value taken from black) leaves us with only
>>>> 4-6 E501's (line too long) in test.
>>>
>>> That is *today* with the existing “new meaningless variable” hacks already
>>> in place.
>> Correct.
>>
>>>
>>>
>>>>
>>>>>
>>>>> That said: I think it is a good idea to add the formatter as a
>>>>> dependency, add a “make test-fixstyle” target and let the willing folks
>>>>> experiment with it for the time being for the regular commits, this will
>>>>> keep the white space changes contained on a per-feature-maintainer basis.
>>>>
>>>> One monster change is supposedly easier to manage from the git side.
>>>> Doing it piecemeal seems to have more problems associated with it.
>>>
>>> If it is done just before LTS branch fork - sure, I won’t mind a single
>>> commit which would both add black as a “make test-fixcheckstyle” and run it
>>> on all the files. With enough warning the folks can avoid having any
>>> inflight work related to it.
>>>
>> See above, 2 commits may make life easier for everyone.
>
> Not so much, imho.
>
>>>
>>>
>>>> As it is today, people frequently make formatting changes in the same
>>>> changeset as something more valuable.
>>>
>>> My experience is that the devs writing C features just want their tests to
>>> work. But I am open to seei the examples.
>> The linting is supposed to accomplish multiple things.
>> 1. Enforce a standard to keep style changes out of git.
>> 2. Catch logic errors. We have short circuited this one to a large degree,
>> by virtue of the fact that we ignore the warnings that aren't trivial to
>> change. ( like 'except:')
>
> Line length does nothing of the above, it’s just a way of enforcing an
> arbitrary preference. For C I can argue it is okay, splitting the line is
> easy. But in my book a language with a significant white space should not
> dictate line length, as this forces a less readable code.
>
>
>>>
>>>> One of the benefits of black, is that it formats the code to be more git
>>>> friendly.
>>>
>>> Define “git friendly” ?
>> Things like sorted imports and extendable lists, dicts, etc. split at points
>> so that git can rebase more changes without needing manual intervention.
>> -from vpp_ip_route import VppIpRoute, VppRoutePath, VppMplsLabel, \
>> - VppIpTable, FibPathProto
>> +from vpp_ip_route import (
>> + VppIpRoute,
>> + VppRoutePath,
>> + VppMplsLabel,
>> + VppIpTable,
>> + FibPathProto,
>> +)
>>
>>
>> - rule_1 = AclRule(is_permit=1, proto=17, ports=1234,
>> - src_prefix=IPv4Network("1.1.1.1/32"),
>> - dst_prefix=IPv4Network("1.1.1.2/32"))
>> + rule_1 = AclRule(
>> + is_permit=1,
>> + proto=17,
>> + ports=1234,
>> + src_prefix=IPv4Network("1.1.1.1/32"),
>> + dst_prefix=IPv4Network("1.1.1.2/32"),
>> + )
>> Here black gave you back an extra 8 characters for not triggering E501. I'm
>> not an expert in black. I've been using it for a month or two on other
>> things, and I don't have (or know) your workflow.
>
> Looks indeed a bit more readable, I agree, thanks for an example. Still not
> enough to do a big white space-only commit at this time. I would not mind
> having that large commit in the week before the 21.10-rc1 is done, though.
>
> But still this should be the same model as in C -and not an automatic format
> at checkin time. To be convinced about that I would need to know that the
> output of the format is always the same if the AST is the same, regardless of
> the white-spacing of the original source.
>
> Also, the question remains how we deal with the “indent update” problem where
> some “fixes” in the indenter cause massive cascading white space only
> changes.... given the python folks tendency to deprecate things from under
> one’s feet, this is actually yet another issue that I haven’t mentioned yet.
>
> And, again, considering the biggest annoyance for everyone today is E501, it
> smells like we are discussing the usage of the nuclear charge for making a
> backyard BBQ...
>
> —a
>
>>> —a
>>>
>>>
>>>>
>>>>>
>>>>> Thoughts ?
>>>>>
>>>>> —a
>>>>>
>>>>> >
>>>>> > Thanks,
>>>>> > Klement
>>>>> >
>>>>> >>> On 2 Dec 2020, at 10:30, Andrew Yourtchenko <ayour...@gmail.com>
>>>>> >>> wrote:
>>>>> >>>
>>>>> >>>
>>>>> >>>> On 2 Dec 2020, at 10:27, Neale Ranns via lists.fd.io
>>>>> >>>> <nranns=cisco....@lists.fd.io> wrote:
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>> Hi Paul,
>>>>> >>>
>>>>> >>> Having to write code to conform to python linting is my number 1
>>>>> >>> annoyance when writing tests. This is my usual hack:
>>>>> >>> e = VppEnum.vl_api_tunnel_encap_decap_flags_t
>>>>> >>> f = e.TUNNEL_API_ENCAP_DECAP_FLAG_ENCAP_COPY_DSCP
>>>>> >>
>>>>> >> +1 E501 specifically being a massive annoyance and having to use the
>>>>> >> exact same hack - if you use the flags multiple times it might forcing
>>>>> >> the somewhat better readability, but then one still has to do f1, f2
>>>>> >> and later combine *them*, which definitely doesn’t help understanding
>>>>> >> of the code by any later reader since now they have to keep these
>>>>> >> mappings in the head.
>>>>> >>
>>>>> >> —a
>>>>> >>
>>>>> >>>
>>>>> >>> I support having an auto-linter. I have no knowledge about what’s
>>>>> >>> available, so I defer to your choice. All I ask is that it works 😉
>>>>> >>> i.e. you don’t have to pepper code with /* *HERE-BE-DRAGONS* */
>>>>> >>>
>>>>> >>> IIUC the plan post 21.01 is to upgrade our default linux distro to
>>>>> >>> 20.04, that brings git 2.25 (at least that’s what my VM has, but
>>>>> >>> maybe I put that there for recent gerrit up-revs…)
>>>>> >>>
>>>>> >>> /neale
>>>>> >>>
>>>>> >>> From: <vpp-dev@lists.fd.io> on behalf of Paul Vinciguerra
>>>>> >>> <pvi...@vinciconsulting.com>
>>>>> >>> Date: Tuesday 1 December 2020 at 23:56
>>>>> >>> To: vpp-dev <vpp-dev@lists.fd.io>
>>>>> >>> Subject: [vpp-dev] replacing make test-checkstyle with black
>>>>> >>>
>>>>> >>> I'd like to propose that we make it easier for everyone by adding
>>>>> >>> black [0] as a pre-commit hook. Black will automatically reformat
>>>>> >>> your file to a git friendly, pep-8 friendly file.
>>>>> >>> For those interested in the details, it moves to a line length of 88,
>>>>> >>> which helps us out with the lengthy VppEnum names we have. We can
>>>>> >>> keep it at 80 if the community objects.
>>>>> >>> I can't do anything about:
>>>>> >>> /vpp/build-root/build-test/src/test_ipsec_esp.py:504:89: E501 line
>>>>> >>> too long (97 > 88 characters)
>>>>> >>>
>>>>> >>> VppEnum.vl_api_tunnel_encap_decap_flags_t.TUNNEL_API_ENCAP_DECAP_FLAG_ENCAP_COPY_DSCP
>>>>> >>> ;)
>>>>> >>>
>>>>> >>> For those who want more details in the changes, see the black code
>>>>> >>> style [1]
>>>>> >>>
>>>>> >>> Saving time around python linting is the #1 request I have had from
>>>>> >>> the community.
>>>>> >>>
>>>>> >>> This is a MASSIVE whitespace change. git blame can ignore whitespace
>>>>> >>> changes starting in git 2.23.
>>>>> >>>
>>>>> >>> The question is whether the community wants to upgrade their version
>>>>> >>> of git to ignore this change with git blame, in exchange for not
>>>>> >>> having to manually lint/fix their files.
>>>>> >>>
>>>>> >>> Thoughts?
>>>>> >>>
>>>>> >>> [0] https://github.com/psf/black
>>>>> >>> [1]
>>>>> >>> https://github.com/psf/black/blob/master/docs/the_black_code_style.md
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>
>>>>> >>
>>>>> >
>>>>> >
>>>>> >
>>>>> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#18236): https://lists.fd.io/g/vpp-dev/message/18236
Mute This Topic: https://lists.fd.io/mt/78647163/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
