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.
>
>
>> - 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.
>
>
> 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:')
>
> 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.
> â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 (#18232): https://lists.fd.io/g/vpp-dev/message/18232
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]
-=-=-=-=-=-=-=-=-=-=-=-
