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 >
