Enable More PyDocStyle Checks

[kaxil]

We use PyDocStyle in pre-commit to enforce docstring style

We use Ruff to enforce docsting style

airflow/pyproject.toml

Lines 47 to 74 in 23b8e83

	extend-select = [
	"I", # Missing required import (auto-fixable)
	"UP", # Pyupgrade
	"RUF100", # Unused noqa (auto-fixable)
	# implicit single-line string concatenation
	"ISC001",
	# We ignore more pydocstyle than we enable, so be more selective at what we enable
	"D101",
	"D106",
	"D2",
	"D3",
	"D400",
	# "D401", # Not enabled by ruff, but we don't want it
	"D402",
	"D403",
	"D412",
	"D419"
	]
	extend-ignore = [
	"D203",
	"D205",
	"D212",
	"D213",
	"D214",
	"D215",
	"E731",
	]

We follow pep257 style (http://www.pydocstyle.org/en/stable/error_codes.html) for checks.

Currently, we ignore the following rules:

The task is to complete the following missing rules:

Missing Docstrings

• D100 | Missing docstring in public module
• D102 | Missing docstring in public method
• D104 | Missing docstring in public package
• [ ] D105 | Missing docstring in magic method : As discussed in https://lists.apache.org/thread/8jbg1dd2lr2cfydtqbjxsd6pb6q2wkc3, Remove D105 from pyproject.toml #38452 - we remove the D105 rule from our checks.
• D107 | Missing docstring in __init__

Whitespace Issues

• D200 | One-line docstring should fit on one line with quotes -- (Add D200 pydocstyle check #11688)
• D202 | No blank lines allowed after function docstring -- (Add D202 pydocstyle check #11032)
• D204 | 1 blank line required after class docstring -- (Add D204 pydocstyle check #11031)
• D205 | 1 blank line required between summary line and description

Docstring Content Issues

• D400 | First line should end with a period
• D401 | First line should be in imperative mood Fix remaining D401 checks  #37256

It would be good if we can enable them one by one -- separate PRs are ok

Let me know if you need any help

[pcandoalmeida]

I've taken:

• D204

Might be an idea @kaxil to add a checklist for all rules needing updating in the initial issue comment. This way potential contributors can know which ones are done and which are still open to do? Just a quality of life idea so we don't have to look at the PRs individually.

[kaxil]

@pcandoalmeida That's a good idea, I updated the description and added a check-box.

[pcandoalmeida]

Hi @kaxil I'm working on D202 now.

[pcandoalmeida]

Morning @kaxil what do you think about adding the Hacktoberfest label to this issue?

[kaxil]

Morning @kaxil what do you think about adding the Hacktoberfest label to this issue?

Good idea again, just added :)

[ktrueda]

Hi I wanna try D205. Can I try this ?

[ktrueda]

I started to fix D205 (1 blank line required between summary line and description).
It's difficult because a lot of docs has not summary line.

For example,

airflow/airflow/configuration.py

Lines 481 to 486 in d86cf37

	def remove_option(self, section, option, remove_default=True):
	"""
	Remove an option if it exists in config from a file or
	default config. If both of config have the same option, this removes
	the option in both configs unless remove_default=False.
	"""

This docs has no summary line and there are a lot of docs like this.
I cannot make summary line for all these docs.
If I deal all docs as summary line, the doc obey D205. But it is not easy to read.

How can I do that? If someone has any idea, please help me.

[kaxil]

@ktrueda You can do 2 things:

1. Add a summary line:

Remove an option if it exists in config from a file or 
default config. 

If both of config have the same option, this removes 
the option in both configs unless remove_default=False.

2. Only update blank line for which docstrings exists. In this case, we won't be able to enable D205 for all of them but when in future we have summary lines, it would make the word easier.

I would prefer (1) though

[ktrueda]

@kaxil Thank you for your help.
I would prefer (1) too. So I'll try that way.

Since I found thousands of docs to fix, I need much times.
Leave it to me.

[potix2]

@kaxil Can I take D200?

[pcandoalmeida]

Hiya 👋🏼 @potix2 I believe D200 is currently being worked on 😃

[potix2]

@pcandoalmeida Yeah, I'm working on it. I'll send a pull request soon.

[potix2]

I'm working on D400.

[peter0083]

I would like to tackle this issue if it hasn't been assigned recently. Thanks! @uranusjr :)

[uranusjr]

Go ahead 👍 Note that the ignore list above is out of date; please find the correct list in the pre-commit config file on main.

[Taragolis]

Just finish calculation other violations of D100-D107

Rule	Total Errors	Uniq Core Modules	Uniq Providers Modules	Uniq Other Modules
D100	767	303	463	1
D102	2202	117	474	1
D104	104	53	402	0
D105	237	55	21	0
D107	1557	127	402	0

Tip

D104 for providers packages should be resolved by change PROVIDER__INIT__PY_TEMPLATE.py.jinja2 IN the dev/breeze/src/airflow_breeze/templates DIRECTORY

If someone interested to look on possible exclusion lists, see attached archive ruff-exclusions.zip

[Taragolis]

In theory we could enable all rules #10742 (comment) but place exclusions into the separate file, e.g. in ruff.toml

extend = "pyproject.toml"

[lint.extend-per-file-ignores]
"airflow/api/auth/backend/kerberos_auth.py" = ["D100", "D107"]
"airflow/api/common/airflow_health.py" = ["D100"]
"airflow/api_connexion/endpoints/config_endpoint.py" = ["D100"]

# Another 1500 lines of exclusions

"airflow/providers/teradata/hooks/teradata.py" = ["D107"]
"airflow/providers/yandex/secrets/lockbox.py" = ["D107"]

"hatch_build.py" = ["D100", "D102"]

[rawwar]

In theory we could enable all rules #10742 (comment) but place exclusions into the separate file, e.g. in ruff.toml

extend = "pyproject.toml"

[lint.extend-per-file-ignores]
"airflow/api/auth/backend/kerberos_auth.py" = ["D100", "D107"]
"airflow/api/common/airflow_health.py" = ["D100"]
"airflow/api_connexion/endpoints/config_endpoint.py" = ["D100"]

# Another 1500 lines of exclusions

"airflow/providers/teradata/hooks/teradata.py" = ["D107"]
"airflow/providers/yandex/secrets/lockbox.py" = ["D107"]

"hatch_build.py" = ["D100", "D102"]

Can we just update it in the current pyproject.toml? I would like to contribute and I'll finish them off one by one

[potiuk]

Can we just update it in the current pyproject.toml? I would like to contribute and I'll finish them off one by one

Oh absolutley. Moving it out is just theorethically possible, but we do not want to do it (or at least we have not decided about it yet. I personally think there is a value in keeping all project settings in a single pyproject.toml.

[rawwar]

Oh absolutley. Moving it out is just theorethically possible, but we do not want to do it (or at least we have not decided about it yet. I personally think there is a value in keeping all project settings in a single pyproject.toml.

I was just trying to do this. But, I see an issue with updating pyproject.tom

one of the module "airflow/api/auth/backend/kerberos_auth.py" has D100 issue and also E402. Since, we are adding separate sections with module as a key in pyproject.toml, that's causing a conflict.

As of now, I only see three other modules causing above conflict:

airflow/security/kerberos.py
airflow/security/permissions.py
airflow/security/utils.py

[potiuk]

We could combine several D*s if we want in one row. I don't think there is a particular reason why we want to keep them in separate sections. @Taragolis ?

[rawwar]

We could combine several D*s if we want in one row. I don't think there is a particular reason why we want to keep them in separate sections. @Taragolis ?

Issue is that same module have D*s and also E*s.

[potiuk]

Issue is that same module have Ds and also Es.

We could combine D*'s and E*s together as well.

[Taragolis]

We could combine several D*s if we want in one row. I don't think there is a particular reason why we want to keep them in separate sections. @Taragolis ?

I don’t think it is even possible not to combine them if we want to enable all of them. Only problem to create issues with all of them

[rawwar]

I don’t think it is even possible not to combine them if we want to enable all of them. Only problem to create issues with all of them

I just raised this draft PR - #37469 . can you please take a look. ruff did ignore multiple lint errors in a single line.

[Taragolis]

And personally not sure about D107

[ferruzzi]

I know this is ancient, but checking back in on it. It appears that the remaining rules on the checklist are no longer ignored. Searching the current pyproject.toml doesn't find D100, D102, D104, and D107. I think maybe we can call this done, or are there new rules we'd like to target now?

[potiuk]

Agree.

[ferruzzi]

@potiuk Sorry, I started a slack thread and did a bit of a deep dive on this but I never followed up here.

Since the "D1" prefix is not enabled, the D100, D102, D104, and D107 don't need to be explicitly excluded. So currently, they are not actually being checked, but it isn't obvious that they aren't and I mistook the fact they aren't excluded to mean they were running.

TLDR: If we want those rules enabled, then this is not actually done.

Proposal:
A new issue (since this one is N years old) and either explicitly exclude those or add a comment in the code indicating we'd like to enable them so people can see them as tasks needing to be done and not make the same mistake I did.

[potiuk]

A new issue (since this one is N years old) and either explicitly exclude those or add a comment in the code indicating we'd like to enable them so people can see them as tasks needing to be done and not make the same mistake I did.

+1