apache
Enable More PyDocStyle Checks
~~We use PyDocStyle in pre-commit to enforce docstring style~~
We use Ruff to enforce docsting style https://github.com/apache/airflow/blob/23b8e839a35e84d57c5cf38b0b21171ac3bd1ecc/pyproject.toml#L47-L74
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, https://github.com/apache/airflow/pull/38452 - we remove the D105 rule from our checks.
- [ ] D107 | Missing docstring in
__init__
Whitespace Issues
- [x] D200 | One-line docstring should fit on one line with quotes -- (https://github.com/apache/airflow/pull/11688)
- [x] D202 | No blank lines allowed after function docstring -- (https://github.com/apache/airflow/pull/11032)
- [x] D204 | 1 blank line required after class docstring -- (https://github.com/apache/airflow/pull/11031)
- [x] D205 | 1 blank line required between summary line and description
Docstring Content Issues
- [x] D400 | First line should end with a period
- [x] D401 | First line should be in imperative mood https://github.com/apache/airflow/issues/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
I've taken:
- [x] 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.
@pcandoalmeida That's a good idea, I updated the description and added a check-box.
Morning @kaxil what do you think about adding the Hacktoberfest label to this issue?
Morning @kaxil what do you think about adding the
Hacktoberfestlabel to this issue?
Good idea again, just added :)
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, https://github.com/apache/airflow/blob/d86cf37a35b69fea806be7610deb332468dfba4b/airflow/configuration.py#L481-L486
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.
@ktrueda You can do 2 things:
- 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.
- 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
@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.
I would like to tackle this issue if it hasn't been assigned recently. Thanks! @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.
status update:
- Haven't forgotten this issue 🐌. I'm still working D205 and would like to ask if there is an automated way to apply all the changes. If not, I will probably switch from my MacBook Air to my Linux which is more powerful which I hope can reduce the wait time when it goes through the pre-commit checks.
many thanks!
Maybe try docformatter? You’d probably want to do this in small batches, maybe even one file at a time, to see how well it works.
Maybe try docformatter? You’d probably want to do this in small batches, maybe even one file at a time, to see how well it works.
sounds good. I will give docformatter a shot. thank you for the tips. @uranusjr. 🙏
@eladkal I gave up D400, because I got too many conflicts. I'm sorry for late reply.
@eladkal sorry I give up on D205. I will let other developers tackle this one 😞
@eladkal, is somebody working here: D400(First line should end with a period)? If not, I'd like to give it a try.
@eladkal @kaxil I took D400, there are a lot of files there 😄 . My question is in this part where we have a long first line
Pre commits (D400) is asking to add a period at the end of the line, It is not the case, I think. Should I just remove the new line and convert it in a single line to avoid have that issue?
Simply removing the newline would trigger another check complaining the line is too long. It would be best to rewrite it and split the sentence into two shorter ones. This particular exampel can be rewritten to
Check metric values are within a certain tolerance.
The metrics are given as SQL expressions, and the tolerance with parameter *days_back*.
Simply removing the newline would trigger another check complaining the line is too long. It would be best to rewrite it and split the sentence into two shorter ones. This particular exampel can be rewritten to
Check metric values are within a certain tolerance. The metrics are given as SQL expressions, and the tolerance with parameter *days_back*.
Hard to automate I am a afraid :(
@potiuk @uranusjr
I counted all the changes that are needed for D400, they are 3150 changes. Finish this in a single PR could be hard. I got an idea. How about to keep ignoring D400 in pre-commits - --add-ignore=D100,D102,D103,D104,D105,D107,D202,D400,D401,D205 and still working in minitaks (100 or 200 files) and send PR fixing what @uranusjr is saying? I could make progress in this way. What do you think?
@edithturn SGTM. The smaller the changes are, the easier it will be to review and merge.
