+1 to what Ash has to say. +1 to enforcing E731 too.
Just wondering what others think of D107 here? Thanks & Regards, Amogh Desai On Tue, Jul 16, 2024 at 6:40 AM Tzu-ping Chung <t...@astronomer.io.invalid> wrote: > +1 to enforcing E731, using lambdas is objectively worse in every way > except you save two lines and a few characters at most. The lambda is only > “needed” in the sense that you can’t do (say) functool.partial due to > variable scoping. A nested function does exactly the same thing. > > -- > Sent from my iPhone > > > On 16 Jul 2024, at 00:14, Ferruzzi, Dennis <ferru...@amazon.com.invalid> > wrote: > > > > I forgot the formatting was going to get stripped on the email, sorry > about that. How do we feel about these two? > > > > > > "E731", Use a method instead of a lambda when declaring a variable, > currently 3 violations > > > > Seems easy enough, just replace a lambda with a getter function but > someone would need to look more into one spot [1] where there is a comment > which seems to say it needs a lambda, but I have not looked into it yet. > > > > "PT019", Use @pytest.mark.usefixtures instead of passing a fixture as a > parameter, currently 268 violations > > > > This one I really just don't know about. Seems like we gain some > clarification vs passing fixtures in as test parameters, which I kind of > like, but the more explicit declaration is pretty clunky. I'm inclined to > say it's more trouble than it is worth? > > > > > > > > To clean up the formatting mess from the original message and update it > with the discussion so far: > > To Discuss: > > > > "E731", Use a method instead of a lambda when declaring a > variable, currently 3 violations > > > > "PT019", Use @pytest.mark.usefixtures instead of passing a > fixture as a parameter, currently 268 violations > > > > To Do: > > > > "D214", Consistent multi-line docstring indentation, currently 0 > violations > > > > "D215", Enforce number of underscores if you use underlines in a > multi-line docstring, currently 0 violations > > "PT004", Fixture does not return anything, add leading > underscore, currently 311 violations > > > > "PT006", @pytest.mark.parametrize names should be a tuple, > currently 1215 violations > > > > "PT007", @pytest.mark.parametrize values should be a tuple, > currently 391 violations > > "PT011", pytest.raises() is too broad, set the match parameter, > currently 153 violations > > "TCH003", stdlib imports which are only used for typechecking go > in to TYPE_CHECKING block, currently 38 violations > > > > To Don't: > > > > "D100", # Docstring at the top of every file. > > > > "D102", # Missing docstring in public method, currently 2473 > violations > > "D103", # Missing docstring in public function, currently 318 > violations > > "D104", # Docstring at the top of every `__init__.py` file. > > "D105", # See > https://lists.apache.org/thread/8jbg1dd2lr2cfydtqbjxsd6pb6q2wkc3 > > "D107", # Docstring in every constructor is unnecessary if the > class has a docstring. > > "D203", # Conflicts with D211. Both can not be enabled. > > "D212", # Conflicts with D213. Both can not be enabled. > > "PT005", # Fixture returns a value, remove leading underscore, > currently 1 violation > > > > > > > > [1] > https://github.com/apache/airflow/blob/d029e77f2fd704bec4f4797b09d54c5c824a8536/dev/perf/scheduler_dag_execution_timing.py#L293 > > > > > > > > - ferruzzi > > > > > > ________________________________ > > From: Kaxil Naik <kaxiln...@gmail.com> > > Sent: Sunday, July 14, 2024 11:30 AM > > To: dev@airflow.apache.org > > Cc: Ferruzzi, Dennis > > Subject: RE: [EXT] More ruff style rules > > > > CAUTION: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > > > > > > > AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur > externe. Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous > ne pouvez pas confirmer l’identité de l’expéditeur et si vous n’êtes pas > certain que le contenu ne présente aucun risque. > > > > > > > > Yup agreed with both: going ahead with new rules and excluding D102 and > D103 > > > >> On Sun, 14 Jul 2024 at 22:17, Jarek Potiuk <ja...@potiuk.com> wrote: > >> > >>> On Sun, Jul 14, 2024 at 6:34 PM Ash Berlin-Taylor <a...@apache.org> > wrote: > >>> > >>> I agree with everything you've identified except these two: > >>> > >>> D102 and D103 aren't worth it for us - there are lots of "public" > >>> functions from classes but we don't need rendered docs for them unless > >> they > >>> are part of the "public python API". High effort to get good doc > strings > >>> for low return on my view. > >>> > >>> Maybe once we have a more defined public python API (i.e. -72) then we > >>> could turn those two on for those files. > >>> > >>> > >> Fully agree with Ash here. > >> > >> > >> > >>> -ash > >>> > >>> On 12 July 2024 21:24:24 BST, "Ferruzzi, Dennis" > >>> <ferru...@amazon.com.INVALID> wrote: > >>>> Hey folks, > >>>> > >>>> There have been a number of projects over the years to enable more and > >>> more style rules in our linters. They serve as a great first project > and > >>> help to standardize code which makes it easier for Future Us to jump > into > >>> sections of the code we otherwise don't usually work in and for new > >>> contributors to ramp up. The current batch are winding down and I > figure > >>> it's time to get a temperature check for another batch. > >>>> > >>>> This is almost certainly going to open a can of worms since python is > >>> inherently a non-prescriptive language and everyone will have their own > >>> preferences but hopefully we can pick a couple that we can agree > wouldn't > >>> hurt to enable. > >>>> > >>>> The easy part of my proposal is that each ignored rule should have a > >>> one-liner comment EITHER marking it as unwanted (and why) or as TODO. > >> I'll > >>> consider that part to fall under lazy consensus and can make that > change > >> at > >>> the end of this discussion. > >>>> > >>>> The trickier part will be deciding which ones are which. Below is a > >> list > >>> of the ones which are currently disabled, a link to the detailed > >>> explanation of the rule, a one-liner explaining the rule for those who > >>> don't want to click, how many violations exist in the codebase right > now > >>> (for a sense of scale), and my personal suggestion on each one. > >>>> > >>>> > >>>> > >>>> "D100<https://docs.astral.sh/ruff/rules/undocumented-public-module/ > >", > >> # > >>> Unwanted: Docstring at the top of every file. > >>>> "D102<https://docs.astral.sh/ruff/rules/undocumented-public-method/ > >", > >> # > >>> TODO: Docstring in every public method; currently 2473 violations > >>>> "D103<https://docs.astral.sh/ruff/rules/undocumented-public-function/ > >>> ", > >>> # TODO: Docstring in every public function; currently 318 violations > >>>> "D104<https://docs.astral.sh/ruff/rules/undocumented-public-package/ > >", > >>> # Unwanted: Docstring in every `__init__.py` file. > >>>> "D105<https://docs.astral.sh/ruff/rules/undocumented-magic-method/>", > # > >>> Unwanted: Docstring in every magic method, See > >>> https://lists.apache.org/thread/8jbg1dd2lr2cfydtqbjxsd6pb6q2wkc3 > >>>> "D107<https://docs.astral.sh/ruff/rules/undocumented-public-init/>", > # > >>> Unwanted: Docstring in every constructor is unnecessary if the class > has > >> a > >>> docstring. > >>>> "D203<https://docs.astral.sh/ruff/rules/one-blank-line-before-class/ > >", > >>> # Unwanted: Conflicts with D211. Both can not be enabled. > >>>> "D212< > https://docs.astral.sh/ruff/rules/multi-line-summary-first-line/ > >>> ", > >>> # Unwanted: Conflicts with D213. Both can not be enabled. > >>>> "D214<https://docs.astral.sh/ruff/rules/section-not-over-indented/>", > # > >>> TODO: Consistent multi-line docstring indentation; currently 0 > violations > >>>> "D215< > >>> https://docs.astral.sh/ruff/rules/section-underline-not-over-indented/ > >>> ", > >>> # TODO: Enforce number of underscores if you use underlines in a > >> multi-line > >>> docstring; currently 0 violations > >>>> "E731<https://docs.astral.sh/ruff/rules/lambda-assignment/>", # > MAYBE?: > >>> Use a method instead of a lambda when declaring a variable; currently 3 > >>> violations > >>>> "TCH003< > >>> https://docs.astral.sh/ruff/rules/typing-only-standard-library-import/ > >>> ", > >>> # MAYBE?: stdlib imports which are only used for type checking go into > >>> TYPE_CHECKING block; currently 38 violations > >>>> "PT004< > >>> > >> > https://docs.astral.sh/ruff/rules/pytest-missing-fixture-name-underscore/ > >>> ", > >>> # TODO: Fixture does not return anything, add leading underscore; > >> currently > >>> 311 violations > >>>> "PT005< > >>> > >> > https://docs.astral.sh/ruff/rules/pytest-incorrect-fixture-name-underscore/ > >>> ", > >>> # Unwanted: Fixture returns a value, remove leading underscore; > >> currently 1 > >>> violation > >>>> "PT006< > >>> https://docs.astral.sh/ruff/rules/pytest-parametrize-names-wrong-type/ > >>> ", > >>> # TODO: @pytest.mark.parametrize names should be a tuple; currently > 1215 > >>> violations > >>>> "PT007< > >>> > https://docs.astral.sh/ruff/rules/pytest-parametrize-values-wrong-type/ > >>> ", > >>> # TODO: @pytest.mark.parametrize values should be a tuple; currently > 391 > >>> violations > >>>> "PT011<https://docs.astral.sh/ruff/rules/pytest-raises-too-broad/>", > # > >>> TODO: pytest.raises() is too broad, set the match parameter; currently > >> 153 > >>> violations > >>>> "PT019< > >>> https://docs.astral.sh/ruff/rules/pytest-fixture-param-without-value/ > >", > >>> # MAYBE?: Use @pytest.mark.usefixtures instead of passing a fixture as > a > >>> parameter; currently 268 violations > >>>> > >>>> > >>>> What do we think? Some seem pretty obvious like D214 which is > disabled > >>> but currently has no violations so we may as well enable it. Some I > >> don't > >>> have strong feelings about but think we may as well enable them. And > >>> others like PT019 I have honestly no clue. I think overall I'd lean > >>> towards "enable unless we have a good reason not to" as a general > >> guideline? > >>>> > >>>> > >>>> > >>>> - ferruzzi > >>> > >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > >
