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