On Wed, Dec 2, 2020 at 1:12 PM Andrew đœ Yourtchenko <ayour...@gmail.com>
wrote:
> 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.
>
> Git doesn't. Gerrit and our workflow does.
>
>
>
>>
>>
>>> - 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.
>
> From the black docs,
Also, as a temporary safety measure, *Black* will check that the
reformatted code still produces a valid AST that is equivalent to the
original. This slows it down. If you're feeling confident, use --fast.
>
> 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.
>
> Mine or pypi libraries?
> 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...
>
> I don't know that I agree. During this discourse, I got an email about a
trailing whitespace in a recent commit of mine. Now I'm going to fix it,
one of you will review and commit it or not, and everyone's time is
wasted. Do we want to pay down the mortgage now or keep paying for years
to come. People live perfectly fine either way.
> â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 (#18237): https://lists.fd.io/g/vpp-dev/message/18237
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]
-=-=-=-=-=-=-=-=-=-=-=-
