Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Even More PyDocStyle Checks #40567

Open
7 of 12 tasks
ferruzzi opened this issue Jul 2, 2024 · 11 comments
Open
7 of 12 tasks

Enable Even More PyDocStyle Checks #40567

ferruzzi opened this issue Jul 2, 2024 · 11 comments
Labels
contributors-workshop Issues that are good for first-time contributor's workshop good first issue kind:documentation

Comments

@ferruzzi
Copy link
Contributor

ferruzzi commented Jul 2, 2024

Description

Followup to #10742

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 and should work towards enabling them:

Missing Docstrings

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

Rough process to fix these

  1. Set up your dev environment [docs]
    0.5. Go to your airflow directory and run ruff check. Write down the number of failures. Mine currently says 8. This step is just to make sure it's working and give you a baseline.

  2. Open /airflow/pyproject.toml

  3. In the ignore list (currently starting on line 301) comment out the rule you intend to work on

  4. In a terminal, go to the airflow directory and run ruff check again. It should now fail with WAY more errors. Mine currently says 863. If the number didn't go way up, something is wrong with your setup.

  5. Have a quick scroll through that list, paying attention to which ones failed because of the rule you just enabled.

  6. Come up with a rough plan to break that down into bites. Maybe pick one directory that has a bunch of failures and start fixing there.

  7. After you have a handful of fixes done: remove your comment (so it is ignored again) and submit a PR with that chunk of changes (see the Contributing docs in step 0)

PLEASE submit these in many small PRs. If you do it all at once it will turn into a 3000-line PR and nobody will review it. Eventually, on the last chunk you'll just remove that line from the ignore list entirely when there are no more violations to fix.

Let me know if you need any help, or ask in the Slack server.

@ferruzzi ferruzzi added kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet labels Jul 2, 2024
@ferruzzi ferruzzi added good first issue contributors-workshop Issues that are good for first-time contributor's workshop and removed kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet labels Jul 3, 2024
@ferruzzi
Copy link
Contributor Author

ferruzzi commented Jul 3, 2024

I included D107 in the list because it was part of the original plan in #10742, but I'd like to argue for dropping it. I don't feel that a docstring in every __init__ method is helpful at all. We're going to just copy/paste something like "Construct the {Class Name} object." a billion times.

@rohithkanchukatla
Copy link

Hi Ferruzzi, Can I take up this issue?

@potiuk
Copy link
Member

potiuk commented Jul 9, 2024

Feel Free.

@ferruzzi
Copy link
Contributor Author

ferruzzi commented Jul 9, 2024

Please do! You don't even need to take the whole thing. In fact, this is much easier to do in small chunks. Fix all the D100 changes in one area, submit the PR, fix another area, submit that pr, etc. then move on to D102 and start over, etc. Small PRs are easier for folks to review and more likely to get merged quickly.

This could (should?) easily be a team project for a bunch of people, just take bites out of it when and where you can.

Feel free to hit me up on Slack if you need a hand getting started.

@ferruzzi
Copy link
Contributor Author

You know what, I'm just going to make the call and drop D107 off this list. If someone wants to do it, they are welcome to but I won't consider it a requirement to close this project. 👍

@ferruzzi
Copy link
Contributor Author

After some discussion on the mailing list it looks like D102 and D103 are minimal benefit, but we do have a few others we'd like to enable and a couple that are still being discussed. I'll update the original description with details.

@dannyl1u
Copy link
Contributor

I can make a PR for:

  • D214 | Consistent multi-line docstring indentation

@dannyl1u
Copy link
Contributor

I can make a PR for:

  • D214 | Consistent multi-line docstring indentation

Update: I believe D214 can be marked as complete. I ran ruff check with the D214 commented out in the ignore list and there were no additional errors. Verified that my environment was working by manually changing the spacing of a couple of function header docstrings to trigger errors.

@dannyl1u
Copy link
Contributor

I can make a PR for:

  • D214 | Consistent multi-line docstring indentation

Update: I believe D214 can be marked as complete. I ran ruff check with the D214 commented out in the ignore list and there were no additional errors. Verified that my environment was working by manually changing the spacing of a couple of function header docstrings to trigger errors.

Same with D215

dannyl1u added a commit to dannyl1u/airflow that referenced this issue Sep 27, 2024
@ferruzzi
Copy link
Contributor Author

ferruzzi commented Oct 7, 2024

Looks like you are correct, I'll check them off on the list here. Can you submit a PR with those two removed from the ignore list so they are enabled, please?

@dannyl1u
Copy link
Contributor

dannyl1u commented Oct 7, 2024

Looks like you are correct, I'll check them off on the list here. Can you submit a PR with those two removed from the ignore list so they are enabled, please?

Done! @ferruzzi
#42802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributors-workshop Issues that are good for first-time contributor's workshop good first issue kind:documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants