Allow explicit select of docstring rule to override configured convention #2606

ngnpope · 2023-02-06T12:07:46Z

I know this is documented...

Setting a convention force-disables any rules that are incompatible with that convention, no matter how they're provided, which avoids accidental incompatibilities and simplifies configuration.

...but it seems a little excessive in some cases.

In particular, convention = "pep257" disables D212 and D213. PEP 257 states:

The summary line may be on the same line as the opening quotes or on the next line.

But I'd like to be a little stricter. I'd like to have convention = "pep257" and force enable D213. Not sure whether this is best as a new configuration option to make it something you have to explicitly opt in to, e.g. force-enable = ["D213"]?

(Using v0.0.241.)

The text was updated successfully, but these errors were encountered:

charliermarsh · 2023-02-06T15:38:37Z

I think this was overly aggressive. We can reconsider. It was done to avoid things like users accidentally enabling both D212 and D213, which led to infinite autofix loops. But we since "solved" that with #2369. So we could instead treat conventions slightly differently.

charliermarsh · 2023-02-06T17:59:58Z

It probably makes sense to treat convention as equivalent to inlining an extend-select.

ngnpope · 2023-02-06T18:37:20Z

Aye, that sounds like how I expected it to work

smackesey · 2023-03-10T20:29:32Z

Came here to say that I added convention = "google" but was unsure how it would interact with my existing set of D ignores. Went looking for answers under convention in the docs but didn't find anything. What's the current behavior?

charliermarsh · 2023-03-10T20:49:12Z

The current behavior is that it turns off any rules that are incompatible with the convention.

So, specifically, convention = "google" is equivalent to ignoring D203, D204, D213, D215, D400, D401, D404, D406, D407, D408, D409, and D413.

So for Dagster, you can remove that first block of (non-google docstring) ignores. That is, I think you'd want:

[tool.ruff]
select = ["D"]
ignore = [
  # (missing public docstrings) These work off of the Python sense of "public", rather than our
  # bespoke definition based off of `@public`. When ruff supports custom plugins then we can write
  # appropriate rules to require docstrings for `@public`.
  "D100",
  "D101",
  "D102",
  "D103",
  "D104",
  "D105",
  "D106",
  "D107",

  ##### TEMPORARY DISABLES

  # (assorted docstring rules) There are too many violations of these to enable
  # right now, but we should enable after fixing the violations.
  "D200",  # (one-line docstring should fit)
  "D205",  # (blank line after summary)
  "D212",  # (multi-line docstring summary should start at 1st line)
  "D415",  # (docstring summary should end with a period)
  "D417",  # (missing arg in docstring)

  # (indent docstring rules) These rules should be enabled when ruff lands
  # on/off style comments, which would allow turning them off for specific
  # docstrings.
  "D207", # (under-indented docstring)
  "D208", # (over-indented docstring)
  "D402", # (first line should not be the function's "signature")
]

[tool.ruff.pydocstyle]
convention = "google"

ssbarnea · 2023-04-29T10:27:37Z

The situation seem to be worse for those deciding to use select = ["ALL"] as apparently you get two warnings that you cannot silence in any way (adding to ignore does not work), neither trying to define convention = "pep257" - even if documentation states that it should work.

warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.

Does anyone know a way to keep using ALL and avoid these confusing warnings?

charliermarsh · 2023-04-29T16:05:27Z

@ssbarnea - Do you mind sharing your configuration? For me, I don't see any warnings with this pyproject.toml:

[tool.ruff]
select = ["ALL"]

[tool.ruff.pydocstyle]
convention = "pep257"

ssbarnea · 2023-05-16T16:19:18Z

@charliermarsh Sure, look at https://github.com/ansible/ansible-compat/blob/main/pyproject.toml#L138 -- apparently I get this warning with current ruff too:

$ ruff .
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.
 
$ ruff --version
ruff 0.0.267

charliermarsh · 2023-05-16T16:27:19Z

@ssbarnea - Thanks, I'll take a look now.

charliermarsh · 2023-05-16T16:31:42Z

I'm not seeing those warnings, need to think on why they could be showing up for you:

ansible-compat on  main via 🐍 v3.8.16
❯ ruff . --no-cache
ansible-compat on  main via 🐍 v3.8.16
❯ ruff --version
ruff 0.0.267

charliermarsh · 2023-05-16T16:33:25Z

Could you share the output of ruff --show-settings ., if you have time?

ssbarnea · 2023-05-16T19:24:26Z

Something is confusing ruff, and apparently is not the cache:

ssbarnea@m1: ~/c/a/ansible-compat chore/test
$ ruff .
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.

ssbarnea@m1: ~/c/a/ansible-compat chore/test
$ ruff --version
ruff 0.0.267

ssbarnea@m1: ~/c/a/ansible-compat chore/test
$ ruff --no-cache .
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.

Result of settings dump https://gist.github.com/ssbarnea/18819983d585ad1ed674e1a2e2b49616

charliermarsh · 2023-05-16T20:41:04Z

Is there any way you have something like a ruff.toml or .ruff.toml or pyproject.toml in the filesystem (under ansible-compat) that's not tracked by Git? That could trigger this message but wouldn't show up with --show-settings.

(Do you not see the warnings when you run --show-settings? Or were they omitted from the settings dump?)

ssbarnea · 2023-05-19T10:11:50Z

You are right, I see the warning without even doing anything:

$ ruff --show-settings
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.
error: Expected at least one path to search for Python files
FAIL: 2

I do not have any other ruff config file in my filesystem other than the git-tracked one at https://github.com/ansible/ansible-lint/blob/main/pyproject.toml#L225-L262

tlambert03 · 2023-06-30T10:38:28Z

just wanna add my +1 to this request. Specifically, I'd love to be able to simply add D417 to numpy docstrings.

[tool.ruff]
extend-select = ["D417"]
[tool.ruff.pydocstyle]
convention = "numpy"

Currently, I do this by manually including the full numpy include/exclude spec just so I can keep D417.

smackesey · 2023-07-16T20:30:16Z

Revisiting to say that IMO the current docs are still too uninformative-- if I read https://beta.ruff.rs/docs/settings/#pydocstyle-convention it is not obvious what this does in terms of select/ignore.

If the content from #2606 (comment) were posted in the docs that would go a ways.

charliermarsh · 2023-07-17T02:51:54Z

For now, I expanded on the documentation in #5819.

alanhdu · 2023-11-09T03:21:24Z

I've been taking a look at this and quite surprised by the behavior. From a code POV, I think the fix should be simple: changing

ruff/crates/ruff_workspace/src/configuration.rs

Lines 891 to 893 in 4760af3

    
           for rule in convention.rules_to_be_ignored() { 
        
               rules.disable(*rule); 
        
           }

to something more like:

 for rule in convention.rules_to_be_ignored() { 
    if !rules.enabled(*rule) {
        rules.disable(*rule);
    }     
 }

which would turn the rules off-by-default but allow users to force override it. My question is whether that would be the right path forward, or if we want something more complicated (e.g. if there are rules that are truly incompatible with a convention, then we can distinguish convention.incompatilbe_rules() from convention.off_by_default_rules(). @charliermarsh -- do you have any guidance on what the desired outcome here is? I'm happy to send a PR either way.

(I also have, uestions aboutf where these rules for the conventions came from -- I would've expected several of them to have been on by default for the numpy convention at least, but I'm happy to move that out of this issue's discussion).

charliermarsh · 2023-11-09T05:00:13Z

@alanhdu -- Thanks for taking a look! I think the fix isn't quite that simple. We probably need some logic like: if the rule wasn't explicitly selected (like --select D200, as opposed to --select D), then we should force it off. For each rules_to_be_ignored, we could check if the rule was included verbatim in any of the select or extend_select selectors?

alanhdu · 2023-11-09T17:42:29Z

Yeah, I agree this is a little trickier than I first thought. I filed #8586 with what I think is a reasonable approach -- would be happy to discuss more about the implementation there!

charliermarsh · 2023-11-09T19:31:02Z

Nice, thanks @alanhdu! Will take a look at that PR later today or tomorrow.

## Summary This fixes #2606 by moving where we apply the convention ignores -- instead of applying that at the very end, e track, we now track which rules have been specifically enabled (via `Specificity::Rule`). If they have, then we do *not* apply the docstring overrides at the end. ## Test Plan Added unit tests to `ruff_workspace` and an integration test to `ruff_cli`

tlambert03 · 2023-11-10T18:54:40Z

hooray!! Thanks @alanhdu!

sorenmc · 2024-08-15T09:04:29Z

For anyone else wanting to add D417 while using numpy convention you have to explicitly include it as

[tool.ruff.lint]
extend-select = ["D", <other selections>, "D417"]

[tool.ruff.lint.pydocstyle]
convention = "numpy"

charliermarsh added configuration Related to settings and configuration core Related to core functionality and removed core Related to core functionality labels Feb 6, 2023

ngnpope mentioned this issue Feb 8, 2023

D417 with numpy docstyle? #2672

Closed

tlambert03 mentioned this issue Feb 15, 2023

add pre-commit config manzt/anywidget#56

Merged

tlambert03 mentioned this issue Oct 27, 2023

ruff not picking up missing docstrings (e.g. D417) pydev-guide/pyrepo-copier#18

Closed

charliermarsh mentioned this issue Nov 9, 2023

D400 should accept question mark as a sentence ender #8020

Closed

alanhdu mentioned this issue Nov 9, 2023

Allow overriding pydocstyle convention rules #8586

Merged

charliermarsh closed this as completed in #8586 Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow explicit select of docstring rule to override configured convention #2606

Allow explicit select of docstring rule to override configured convention #2606

ngnpope commented Feb 6, 2023

charliermarsh commented Feb 6, 2023

charliermarsh commented Feb 6, 2023

ngnpope commented Feb 6, 2023

smackesey commented Mar 10, 2023 •

edited

Loading

charliermarsh commented Mar 10, 2023

ssbarnea commented Apr 29, 2023 •

edited

Loading

charliermarsh commented Apr 29, 2023

ssbarnea commented May 16, 2023

charliermarsh commented May 16, 2023

charliermarsh commented May 16, 2023

charliermarsh commented May 16, 2023

ssbarnea commented May 16, 2023

charliermarsh commented May 16, 2023

ssbarnea commented May 19, 2023

tlambert03 commented Jun 30, 2023

smackesey commented Jul 16, 2023 •

edited

Loading

charliermarsh commented Jul 17, 2023 •

edited

Loading

alanhdu commented Nov 9, 2023 •

edited

Loading

charliermarsh commented Nov 9, 2023

alanhdu commented Nov 9, 2023

charliermarsh commented Nov 9, 2023

tlambert03 commented Nov 10, 2023

sorenmc commented Aug 15, 2024

Allow explicit select of docstring rule to override configured convention #2606

Allow explicit select of docstring rule to override configured convention #2606

Comments

ngnpope commented Feb 6, 2023

charliermarsh commented Feb 6, 2023

charliermarsh commented Feb 6, 2023

ngnpope commented Feb 6, 2023

smackesey commented Mar 10, 2023 • edited Loading

charliermarsh commented Mar 10, 2023

ssbarnea commented Apr 29, 2023 • edited Loading

charliermarsh commented Apr 29, 2023

ssbarnea commented May 16, 2023

charliermarsh commented May 16, 2023

charliermarsh commented May 16, 2023

charliermarsh commented May 16, 2023

ssbarnea commented May 16, 2023

charliermarsh commented May 16, 2023

ssbarnea commented May 19, 2023

tlambert03 commented Jun 30, 2023

smackesey commented Jul 16, 2023 • edited Loading

charliermarsh commented Jul 17, 2023 • edited Loading

alanhdu commented Nov 9, 2023 • edited Loading

charliermarsh commented Nov 9, 2023

alanhdu commented Nov 9, 2023

charliermarsh commented Nov 9, 2023

tlambert03 commented Nov 10, 2023

sorenmc commented Aug 15, 2024

smackesey commented Mar 10, 2023 •

edited

Loading

ssbarnea commented Apr 29, 2023 •

edited

Loading

smackesey commented Jul 16, 2023 •

edited

Loading

charliermarsh commented Jul 17, 2023 •

edited

Loading

alanhdu commented Nov 9, 2023 •

edited

Loading