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
> >
>
