-
Notifications
You must be signed in to change notification settings - Fork 3k
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
dev(ingest): move from isort to ruff #12364
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
I see other references to Should we fully remove them? |
@sgomezvillamor Those are separate modules. I am doing this right now only for |
@@ -12,6 +10,7 @@ | |||
import traceback | |||
import unittest.mock | |||
import uuid | |||
from datahub.utilities._markupsafe_compat import MARKUPSAFE_PATCHED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually a pretty dangerous change - we're applying patches to third-party libraries, so we need to make sure that those patches get applied first. so moving the import down is not actually safe.
we'll need to use custom isort sections for this https://docs.astral.sh/ruff/settings/#lint_isort_sections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also note that this is only required for metadata-ingestion
- this specific config does not need to be copied across to every module
Took some hit and trials to come as close to original sorting but minimum these 4 changes were still made.
Checklist