+1 to not include D102, D103. +1 to enforcing E731. (I just introduced this in one of my latest PRs 🤦‍♂️ I will fix it) +1 to not-enforcing D107.
I actually think we could enforce TCH003. This kind of pattern seems to be already applied to the 3rd party library. It could apply to the standard library unless we have a good reason not to. For PT004 and PT005, I think we probably should either enforce both or exclude both for less confusiton. For PT019, I think it’s something we should enforce. (I also love PT006, PT007, and PT011, but yep, they’re already in TODO.) Best, Wei > On Jul 17, 2024, at 3:31 PM, Amogh Desai <amoghdesai....@gmail.com> wrote: > > +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 >> >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org For additional commands, e-mail: dev-h...@airflow.apache.org
