> On 2 Dec 2020, at 20:07, Paul Vinciguerra <pvi...@vinciconsulting.com> wrote:
>
>
>
>
>> 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.
Cherry-picks are git concepts, so gerrit is not relevant for the purposes of
backporting.
And still I don’t see how dependent changes would be helpful in this case.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> - 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.
Now if they get the diffs and merges based on the AST, I would get curious.
(cf: coccinelle from Inria)
>
>>>
>>> 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?
The overall ecosystem attitude. It’s like having a great fancy knife, except
with very soft steel, so one has to spend half the time sharpening it. I am not
a language geek, I just want to get my tests to run. That said: the said
attitude actually pushes the whole bandwagon, even kicking and screaming, so as
a result I can package the entire python make test into a single tarball with
pyoxidizer, with the only dependency being libc. So there are advantages, too.
I don’t know specifically about your libs, so can’t say.
>>> 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.
Default config in Ubuntu highlights it in bright red background, so it looks
ugly.
It also looks quite prominent in gerrit review as well. If your tooling doesn’t
deal with it, you might want to enhance the tooling.
> Now I'm going to fix it, one of you will review and commit it or not, and
> everyone's time is wasted.
my conclusion from the above story is that:
1) you didn’t have a second look at the diff before you submitted, or if you
did, the environment you have is not showing the trailing spaces
2) the +2’er didn’t look well enough at what they +2’d
So doing a little walk of shame is more than appropriate in this case, in my
opinion. It’s not the time “wasted”, it’s the time which was borrowed by taking
a shortcut which now is due to be repaid. So complaining about it is not an
appropriate course of action.
That said: we can probably add a trivial check (and maybe even fix) for
trailing spaces in diff into checkstyle; but again it’s not a nuclear option
like a new reformat, and if you don’t beat me to it I might make it in a few
days.
But I think in the mere reading by the community about this incident in this
exchange maybe 10x time was spent than simply quietly getting it fixed.
> Do we want to pay down the mortgage now or keep paying for years to come.
> People live perfectly fine either way.
Getting affordable housing to begin with avoids the whole dilemma.
—a
>>> —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 (#18238): https://lists.fd.io/g/vpp-dev/message/18238
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]
-=-=-=-=-=-=-=-=-=-=-=-
