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

Automatically detect if non-lazy logging interpolation is used #24910

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 8, 2022

We used to have pylint check that was preventing it but since
we have no pylint, we need to do some intelligent guessing
based on AST of the python code.


^ 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.

@potiuk
Copy link
Member Author

potiuk commented Jul 8, 2022

Finally I got to doing it:

Screenshot 2022-07-08 at 03 39 27

@potiuk
Copy link
Member Author

potiuk commented Jul 8, 2022

Maybe not perfect, but I think it should catch vast majority of cases when f-strings are used in logging.

STATIC_CODE_CHECKS.rst Outdated Show resolved Hide resolved
We used to have pylint check that was preventing it but since
we have no pylint, we need to do some intelligent guessing
based on AST of the python code.
@potiuk potiuk force-pushed the add-pre-commit-for-non-lazy-logging-interpolation branch from 4fa5a1e to 43375e6 Compare July 8, 2022 08:42
@potiuk potiuk merged commit acaa063 into apache:main Jul 8, 2022
@potiuk potiuk deleted the add-pre-commit-for-non-lazy-logging-interpolation branch July 8, 2022 11:02
@josh-fell
Copy link
Contributor

Ah great minds! I was actually working on implementing the flake8-logging-format extension based on Kaxil's comment in #23597. It's definitely a more intense check beyond f-strings, but I'm happy to stop working on this if the check here satisfies needs.

@potiuk
Copy link
Member Author

potiuk commented Jul 8, 2022

Ahh... :). Maybe the flake8 might be better. My solution is really a quick&dirty hack which detects likely vast majority of the uses of logging but not all of them (and it's a bit custom - making assumption on our use of our conventions (for example that our logger is set as "log" attribute). But more "complete" and fool-proof reusable solution as flake8 extension is definitely preferrable, so if you want to do it, publish and share with others, I am more than happy to replace my "dirty script" with it ).

@josh-fell
Copy link
Contributor

Ahh... :). Maybe the flake8 might be better. My solution is really a quick&dirty hack which detects likely vast majority of the uses of logging but not all of them (and it's a bit custom - making assumption on our use of our conventions (for example that our logger is set as "log" attribute). But more "complete" and fool-proof reusable solution as flake8 extension is definitely preferrable, so if you want to do it, publish and share with others, I am more than happy to replace my "dirty script" with it ).

Tossed #24931 and #24933 out there for flake8-logging-format. Would love feedback if folks are willing 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants