Skip to content

Conversation

@vincbeck
Copy link
Contributor

@vincbeck vincbeck commented May 8, 2023

Enable D400 pydocstyle check. Part of #10742.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI labels May 8, 2023
@vincbeck vincbeck requested a review from XD-DENG as a code owner May 8, 2023 17:00
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:Scheduler including HA (high availability) scheduler labels May 8, 2023
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every instance of the change is correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it’d be a good idea to make Pydocstyle skip the migrations directory altogether, since the docstring auto-generated by alembic by default does not follow D400. Or maybe we need to configure alembic to emit a different migration file template.

@uranusjr
Copy link
Member

uranusjr commented May 10, 2023

Would it be possible to split migrations and providers into another PR, or does everything must be fixed once? This is really a lot.

@vincbeck
Copy link
Contributor Author

Would it be possible to split migrations and providers into another PR, or does everything must be fixed once? This is really a lot.

I can split. Let me do that. That'll definitely help the review

@vincbeck
Copy link
Contributor Author

Hold on. No, that wont work. Static checks will fail unless we specify we want to run the D400 check only on specific files/folders. I am not sure it is possible

@vincbeck
Copy link
Contributor Author

Or one solution would be to make the modifications without enabling the check. Let me know, what you think

@uranusjr
Copy link
Member

Static checks will fail unless we specify we want to run the D400 check only on specific files/folders.

Yeah that’s what I suspected as well 😞

make the modifications without enabling the check

This sounds like a workable alternative. I think we did that when we enabled some other checks previously. Make the changes gradually, and enable the check only after all (or most of) the changes are made. IIRC @Bowrna worked on some of the changes back then and may be able to share some experience.

@Bowrna
Copy link
Contributor

Bowrna commented May 11, 2023

yes let me check the changes i have done and tell you. for testing purpose, i remember i enabled it to run for specific files alone before enabling it for all the files.

@vincbeck
Copy link
Contributor Author

yes let me check the changes i have done and tell you. for testing purpose, i remember i enabled it to run for specific files alone before enabling it for all the files.

Oh if that's possible, that would be lovely!

@vincbeck
Copy link
Contributor Author

@Bowrna Did you find it? Otherwise, no big deal, I'll split this PR without enabling the check at first

@Bowrna
Copy link
Contributor

Bowrna commented May 15, 2023

hi @vincbeck sorry for not updating here quickly. When I made a change for the pydocstyle I had a file named .pre-commit-config.yaml where I can pass the parameter in exclude to ignore running the pydocstyle. I couldn't find something alike in your commit and therefore I didn't update. please check the screenshot and see if it was useful for you. this one allows to exclude but not run on specific ones.

Screenshot 2023-05-15 at 9 01 20 PM

Reference PR: https://github.com/apache/airflow/pull/24221/files#

@vincbeck
Copy link
Contributor Author

Thanks for the answer! I see, this is actually what I was planning to do. Thanks :)

@vincbeck
Copy link
Contributor Author

I created a PR to address only Airflow core: #31297. Other PRs will come along later

@vincbeck vincbeck marked this pull request as draft May 15, 2023 20:05
@vincbeck
Copy link
Contributor Author

I converted this PR in draft since we do not want to merge it but I'll keep it open to track the overall work on D400 check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:CLI area:Scheduler including HA (high availability) scheduler provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants