> 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. > >> >> - 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. > >> >> 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. > 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. > One of the benefits of black, is that it formats the code to be more git > friendly. Define “git friendly” ? —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 (#18229): https://lists.fd.io/g/vpp-dev/message/18229 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] -=-=-=-=-=-=-=-=-=-=-=-
