+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
