From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B626BA0032; Mon, 12 Sep 2022 16:06:41 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 65022400D4; Mon, 12 Sep 2022 16:06:41 +0200 (CEST) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mails.dpdk.org (Postfix) with ESMTP id E3D9E4003C for ; Mon, 12 Sep 2022 16:06:39 +0200 (CEST) Received: by mail-pj1-f49.google.com with SMTP id i15-20020a17090a4b8f00b0020073b4ac27so8251802pjh.3 for ; Mon, 12 Sep 2022 07:06:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=fcohDhWbKqtHWt0EXiYyMY1hw+3DFZruvtoK1+f/XwI=; b=SyaoRPkiYkAWCjCQe5jNeBq1bFP/868ICFjdVr/xy8jqC8c5t3NGICkCTXb5ezvhFB CHZ7n/AMGEymrFJxvYpwlVdOEe8QPOF62ddwi/7jMwD/90AjOx0EPa7SxnxDaFGWTxe7 HcgLZyQ4yRQ0TUcFHGhONt90X3bgFLnZUeo6k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=fcohDhWbKqtHWt0EXiYyMY1hw+3DFZruvtoK1+f/XwI=; b=zdiEKdrbETCuzSFnoYx1+2aiLyESXywdbQH8i6y2LXCGPk1exq5dkQBn0+pPmH/fLd QTbRZFamHGAz+jlNuz6iNjb5bbqzNKPriFbn1oBfEpzj0TonGKmX0gUWzyO/4oQNrelQ Xlax+UiakKxxLDwdmirRJyKvY+MS0JnEE7+HNylkZD05IdRWL9ZXMWfNc5+7IkX8kd7r tkQptrvUnLli9CO5uFBw4WTfbTYc3lme/VvGbCY0ma2rTnvxfc+4kNSlHfo1DP+6znnb NG4kQb9+Al9wjuCoHwgiPy2zuGcJykAzk6vxMCV3MRRUh0iQAAdp4jN+tNXzi/F7nhJV UNHQ== X-Gm-Message-State: ACgBeo3f1TGS2UA8UksgUnDrPDHdFWcpCHmlUb4yLslx1AidaKXrsAF9 Uy/AtGt39HL1Uzjl0wbvjyB9o/tsksRwc5Zbu8XMaw== X-Google-Smtp-Source: AA6agR4k3JFxmJPkuJ6Cgq66DZr5YWbEzTtMo3GT6szb5KC1vtdiwzlCOrNowEN1sYbsGHLCos8b7YfKKOjertUDIhQ= X-Received: by 2002:a17:90b:17cf:b0:202:95a2:e310 with SMTP id me15-20020a17090b17cf00b0020295a2e310mr16240398pjb.76.1662991598962; Mon, 12 Sep 2022 07:06:38 -0700 (PDT) MIME-Version: 1.0 References: <20220728100044.1318484-1-juraj.linkes@pantheon.tech> <20220729105550.1382664-1-juraj.linkes@pantheon.tech> <20220729105550.1382664-2-juraj.linkes@pantheon.tech> <3680ad0ceb5944e6b5050d5f7b9c9599@pantheon.tech> In-Reply-To: From: Owen Hilyard Date: Mon, 12 Sep 2022 10:06:03 -0400 Message-ID: Subject: Re: [PATCH v4 1/9] dts: add project tools config To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Bruce Richardson , "thomas@monjalon.net" , "david.marchand@redhat.com" , "ronan.randles@intel.com" , "Honnappa.Nagarahalli@arm.com" , "lijuan.tu@intel.com" , "dev@dpdk.org" Content-Type: multipart/alternative; boundary="0000000000008567a305e87b6706" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000008567a305e87b6706 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > E203 - whitespace before =E2=80=98,=E2=80=99, =E2=80=98;=E2=80=99, or =E2= =80=98:=E2=80=99 > E266 - too many leading =E2=80=98#=E2=80=99 for block comment > E501 - line too long > E731 - do not assign a lambda expression, use a def > C0111 - Missing %s docstring > F0401 - Unable to import %s E203, E266 and E501 were disabled due to pylama fighting with the autoformatters, so I decided to let the autoformatters win. I think that C0111 was suppressed because this set of suppressions was from mainline DTS and that has a lot of functions without documentation. F0401 is disabled due to dependencies on TRex vendored python libraries, since those will not be possible to import inside of the container. I don't remember why E731 is set, but it may be due to the rte flow rule generator I wrote for mainline DTS, which makes use of lambdas extensively to enable lazy evaluation, so that DTS doesn't need to keep ~2 billion rules in memory. On Fri, Sep 9, 2022 at 10:13 AM Juraj Linke=C5=A1 wrote: > > > > -----Original Message----- > > From: Bruce Richardson > > Sent: Friday, September 9, 2022 3:53 PM > > To: Juraj Linke=C5=A1 > > Cc: thomas@monjalon.net; david.marchand@redhat.com; > > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com; > > ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org > > Subject: Re: [PATCH v4 1/9] dts: add project tools config > > > > On Fri, Sep 09, 2022 at 01:38:33PM +0000, Juraj Linke=C5=A1 wrote: > > > > > > > > > > -----Original Message----- > > > > From: Bruce Richardson > > > > Sent: Wednesday, September 7, 2022 6:17 PM > > > > To: Juraj Linke=C5=A1 > > > > Cc: thomas@monjalon.net; david.marchand@redhat.com; > > > > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com; > > > > ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org > > > > Subject: Re: [PATCH v4 1/9] dts: add project tools config > > > > > > > > On Fri, Jul 29, 2022 at 10:55:42AM +0000, Juraj Linke=C5=A1 wrote: > > > > > .gitignore contains standard Python-related files. > > > > > > > > > > Apart from that, add configuration for Python tools used in DTS: > > > > > Poetry, dependency and package manager Black, formatter Pylama, > > > > > static analysis Isort, import sorting > > > > > > > > > > .editorconfig modifies the line length to 88, which is the defaul= t > > > > > Black uses. It seems to be the best of all worlds. [0] > > > > > > > > > > [0] > > > > > https://black.readthedocs.io/en/stable/the_black_code_style/curre= n > > > > > t_st > > > > > yle.html#line-length > > > > > > > > > > Signed-off-by: Owen Hilyard > > > > > Signed-off-by: Juraj Linke=C5=A1 > > > > > > > > Thanks for the work on this. Some review comments inline below. > > > > > > > > /Bruce > > > > > > > > > --- > > > > > dts/.editorconfig | 7 + > > > > > dts/.gitignore | 14 ++ > > > > > dts/README.md | 15 ++ > > > > > dts/poetry.lock | 474 > > > > +++++++++++++++++++++++++++++++++++++++++++++ > > > > > dts/pylama.ini | 8 + > > > > > dts/pyproject.toml | 43 ++++ > > > > > 6 files changed, 561 insertions(+) create mode 100644 > > > > > dts/.editorconfig create mode 100644 dts/.gitignore create mode > > > > > 100644 dts/README.md create mode 100644 dts/poetry.lock create > > > > > mode 100644 dts/pylama.ini create mode 100644 dts/pyproject.toml > > > > > > > > > > diff --git a/dts/.editorconfig b/dts/.editorconfig new file mode > > > > > 100644 index 0000000000..657f959030 > > > > > --- /dev/null > > > > > +++ b/dts/.editorconfig > > > > > @@ -0,0 +1,7 @@ > > > > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2022 > > > > > +PANTHEON.tech s.r.o. > > > > > +# See https://editorconfig.org/ for syntax reference. > > > > > +# > > > > > + > > > > > +[*.py] > > > > > +max_line_length =3D 88 > > > > > > > > It seems strange to have two different editorconfig settings in > > > > DPDK. Is there a reason that: > > > > a) we can't use 79, the current DPDK default and recommended length > by > > > > pycodestyle? Or alternatively: > > > > b) change all of DPDK to use the 88 setting? > > > > > > > > Also, 88 seems an unusual number. How was it chosen/arrived at? > > > > > > > > > > The commit message contains a link to Black's documentation where the= y > > explain it: > > > https://black.readthedocs.io/en/stable/the_black_code_style/current_s= t > > > yle.html#line-length > > > > > > Let me know what you think about it. I think it's reasonable. I'll > move the > > config to the top level .editorconfig file. > > > > > > > I have no objection to moving this to the top level, but others may lik= e > to keep > > our python style as standard. Realistically I see three choices here: > > > > 1. Force DTS to conform to existing DPDK python style of 79 characters > 2. Allow > > DTS to use 88 chars but the rest of DPDK to keep with 79 chars 3. Allow > all of > > DPDK to use 88 chars. > > > > Of the 3, I like relaxing the 79/80 char limit so #3 seems best to me a= s > you > > suggest. However, I'd wait a few days for a desenting opinion before I'= d > do a > > new patchset revision. :-) > > > > Ok, I'll wait. > > > /Bruce > > --0000000000008567a305e87b6706 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
> E= 203 - whitespace before =E2=80=98,=E2=80=99, =E2=80=98;=E2=80=99, or =E2=80= =98:=E2=80=99
> E266 - too many leading =E2=80=98#=E2=80=99 for block com= ment
> E501 - line too long
> E731 - do not assign a lambda expression,= use a def
> C0111 - Missing %s docstring
> F0401 - Unable to import %s=

E203, E266 and E50= 1 were disabled due to pylama fighting with the autoformatters, so I decide= d to let the autoformatters win. I think that=C2=A0C0111 was suppressed bec= ause this=C2=A0set of suppressions was from mainline DTS and that has a lot= of functions without documentation.=C2=A0F0401 is disabled due to dependen= cies on TRex vendored python libraries, since=C2=A0those will not be possib= le to import inside of the container. I don't remember why=C2=A0E731 is= set, but it may be due to the rte flow rule generator I wrote for mainline= DTS, which makes use of lambdas extensively to enable lazy evaluation, so = that DTS doesn't need to keep ~2 billion rules in memory.



On Fri, Sep 9, 2022 at 10:13 AM Jur= aj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:


> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, September 9, 2022 3:53 PM
> To: Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech>
> Cc: thomas@mo= njalon.net; david.marchand@redhat.com;
> ronan.ran= dles@intel.com; Honnappa.Nagarahalli@arm.com;
> ohilyard@iol= .unh.edu; liju= an.tu@intel.com; dev@= dpdk.org
> Subject: Re: [PATCH v4 1/9] dts: add project tools config
>
> On Fri, Sep 09, 2022 at 01:38:33PM +0000, Juraj Linke=C5=A1 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Wednesday, September 7, 2022 6:17 PM
> > > To: Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech>
> > > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> > > oh= ilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> > > Subject: Re: [PATCH v4 1/9] dts: add project tools config > > >
> > > On Fri, Jul 29, 2022 at 10:55:42AM +0000, Juraj Linke=C5=A1 = wrote:
> > > > .gitignore contains standard Python-related files.
> > > >
> > > > Apart from that, add configuration for Python tools use= d in DTS:
> > > > Poetry, dependency and package manager Black, formatter= Pylama,
> > > > static analysis Isort, import sorting
> > > >
> > > > .editorconfig modifies the line length to 88, which is = the default
> > > > Black uses. It seems to be the best of all worlds. [0]<= br> > > > >
> > > > [0]
> > > > https://black.= readthedocs.io/en/stable/the_black_code_style/curren
> > > > t_st
> > > > yle.html#line-length
> > > >
> > > > Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> > > > Signed-off-by: Juraj Linke=C5=A1 <juraj.linkes@panth= eon.tech>
> > >
> > > Thanks for the work on this. Some review comments inline bel= ow.
> > >
> > > /Bruce
> > >
> > > > ---
> > > >=C2=A0 dts/.editorconfig=C2=A0 |=C2=A0 =C2=A07 +
> > > >=C2=A0 dts/.gitignore=C2=A0 =C2=A0 =C2=A0|=C2=A0 14 ++ > > > >=C2=A0 dts/README.md=C2=A0 =C2=A0 =C2=A0 |=C2=A0 15 ++ > > > >=C2=A0 dts/poetry.lock=C2=A0 =C2=A0 | 474
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >=C2=A0 dts/pylama.ini=C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A08= +
> > > >=C2=A0 dts/pyproject.toml |=C2=A0 43 ++++
> > > >=C2=A0 6 files changed, 561 insertions(+)=C2=A0 create m= ode 100644
> > > > dts/.editorconfig=C2=A0 create mode 100644 dts/.gitigno= re=C2=A0 create mode
> > > > 100644 dts/README.md=C2=A0 create mode 100644 dts/poetr= y.lock=C2=A0 create
> > > > mode 100644 dts/pylama.ini=C2=A0 create mode 100644 dts= /pyproject.toml
> > > >
> > > > diff --git a/dts/.editorconfig b/dts/.editorconfig new = file mode
> > > > 100644 index 0000000000..657f959030
> > > > --- /dev/null
> > > > +++ b/dts/.editorconfig
> > > > @@ -0,0 +1,7 @@
> > > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c)= 2022
> > > > +PANTHEON.tech s.r.o.
> > > > +# See https://editorconfig.org/ for syntax referen= ce.
> > > > +#
> > > > +
> > > > +[*.py]
> > > > +max_line_length =3D 88
> > >
> > > It seems strange to have two different editorconfig settings= in
> > > DPDK. Is there a reason that:
> > > a) we can't use 79, the current DPDK default and recomme= nded length by
> > >=C2=A0 =C2=A0 pycodestyle? Or alternatively:
> > > b) change all of DPDK to use the 88 setting?
> > >
> > > Also, 88 seems an unusual number. How was it chosen/arrived = at?
> > >
> >
> > The commit message contains a link to Black's documentation w= here they
> explain it:
> > https://black.readth= edocs.io/en/stable/the_black_code_style/current_st
> > yle.html#line-length
> >
> > Let me know what you think about it. I think it's reasonable.= I'll move the
> config to the top level .editorconfig file.
> >
>
> I have no objection to moving this to the top level, but others may li= ke to keep
> our python style as standard. Realistically I see three choices here:<= br> >
> 1. Force DTS to conform to existing DPDK python style of 79 characters= 2. Allow
> DTS to use 88 chars but the rest of DPDK to keep with 79 chars 3. Allo= w all of
> DPDK to use 88 chars.
>
> Of the 3, I like relaxing the 79/80 char limit so #3 seems best to me = as you
> suggest. However, I'd wait a few days for a desenting opinion befo= re I'd do a
> new patchset revision. :-)
>

Ok, I'll wait.

> /Bruce

--0000000000008567a305e87b6706--