On Fri, Sep 3, 2021 at 9:26 AM Aaron Conole <[email protected]> wrote: > > Frode Nordahl <[email protected]> writes: > > > On Thu, Sep 2, 2021 at 4:04 PM Aaron Conole <[email protected]> wrote: > >> > >> Hi Frode (et. al.), > >> > >> Frode Nordahl writes: > >> > >> > The checkpatch script will hang forever on line 282 of a otherwise > >> > valid patch [0]. While the root of the issue is probably > >> > somewhere between the regex itself and the Python re > >> > implementation, this patch provides an early return to avoid this > >> > specific issue. > >> > > >> > Also fix the docstring for the if_and_for_end_with_bracket_check > >> > function. > >> > > >> > 0: > >> > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > >> > Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com> > >> > --- > >> > >> Sorry, I was on PTO when this patch came in and missed it. I don't > >> agree with your proposed fix. It's a bit too conditional (and I was > >> able to build a test-case that breaks it anyway). > >> > >> I see it's already been applied to OVN, but I plan to propose the > >> following for OVS repository (and suggest a similar change to OVN). > >> > >> The change should be safe because we already validate that we are only > >> in a for/if/while block, so the early bail to True should be okay. > >> > >> WDYT? > > > > No worries, I had to propose something as apparently I was clogging up > > the CI, which is not nice :-) Your approach does look more generally > > useful indeed and I would have no issue with having mine replaced by > > it. > > Cool! > > > However, it does end up with the opposite verdict on the line in > > question, was that intentional? From my reading of the test, it checks > > that when there is room for a bracket at the end of a line for a > > conditional control block it should be there, does that not mean we > > can put it on the next line when there is not room? > > I'm not sure what you mean. > > In Documentation/internals/contributing/coding-style.rst > Section: Statements, I don't see anything about putting the brace on a > separate line. So, I think such error case is correctly flagged. Maybe > there could be an exception placed in the coding style - not sure. It's > a different discussion, though. > > I'll propose this patch formally for OVS. I can also submit the patch > for OVN, if you think that makes sense.
+1 for proposing the patch to OVN. It'd be good for OVN checkpatch to be consistent with ovs. Numan > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
