Apply flake8-logging-format changes to core #24977
Conversation
boring-cyborg
bot
added
area:API
josh-fell
force-pushed
the
flake8-logging-format-core
branch
from
f488c96
to
94bcc1c
Compare
josh-fell
removed
provider:google
| @@ -106,7 106,7 @@ def create_db(self): | |||
| log.info(c.LOGMSG_INF_SEC_ADD_DB) | |||
| super().create_db() | |||
| except Exception as e: | |||
| log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) | |||
| log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001 | |||
There was a problem hiding this comment.
Should we extract a common function doing the logging? Then we could ignore just once.
log_error(c.LOGMSG_ERR_SEC_CREATE_DB, e)
There was a problem hiding this comment.
or:
log_message(Logging.WARNING,c.LOGMSG_ERR_SEC_CREATE_DB, e)
To account for different log levels? That would also allow to make it performant with "if log.level< level)"
There was a problem hiding this comment.
I added the ignore because it was FAB and I assume we just copy directly without making modifications, but I could be wrong. I was thinking adding the ignores would be the least invasive.
There was a problem hiding this comment.
The common logging function is a cool and intriguing idea though. Let's see what I can cook up.
There was a problem hiding this comment.
I thought sqa/manager is ours :)
There was a problem hiding this comment.
Because if it is FAB, then I'd simply exclude the whole file
There was a problem hiding this comment.
Seems like FAB generally is a mix of both. It's copied and then modified accordingly.
There was a problem hiding this comment.
Yeah. Seen that In SecurityManager. 🤯
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
josh-fell
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
Related: #23597 #24910
This is some pre-work for hopefully/maybe including the flake8-logging-format extension in CI. There are roughly 88 file changes so splitting these up into smaller chunks.
After the initial sweeps are completed we can do a final pass when formally implementing the extension in CI.
I'm not entirely certain if there are impacts to useful info that will be missed with log handlers but would love feedback on general patterns.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.