[airflow] add autofix to AIR302 ProviderName #17038
Conversation
|
this branch based on #16965 |
|
Lee-W
force-pushed
the
autofix-AIR303
branch
2 times, most recently
from
1478e22 to
f9e97f0
Compare
|
(I'll avoid reviewing this PR for now as it's in draft and it's based on a different branch; the diff includes code from both branches. Once the dependent PR is merged, we can move this out of draft and I can review it.) |
dhruvmanila
added
fixes
Lee-W
force-pushed
the
autofix-AIR303
branch
2 times, most recently
from
410c3e9 to
bc93544
Compare
|
@dhruvmanila This PR is ready now 🙌 (after all the merging and rebasing 💦) |
You've put this back into draft, is that intentional? |
|
Yep, it is. Found an issue after a few more rebasing. Was trying to fix it but couldn't do it on time. 🥲 |
Lee-W
changed the title
| }, | ||
| ["airflow", "sensors", "s3_key_sensor", "S3KeySensor"] => Replacement::ProviderName{ | ||
| ["airflow", "operators", "google_api_to_s3_transfer", "GoogleApiToS3Operator"] => { | ||
| Replacement::ProviderName { |
There was a problem hiding this comment.
Hey @ntBre, a quick question. I tried to apply autofix to all the similar symbol changes, but only a few of them can be fixed. Do you know what might be missing?
There was a problem hiding this comment.
I might be missing some context here. What kind of autofix were you trying to apply? Or what change were you trying to make?
There was a problem hiding this comment.
for example, if the user is using
from airflow.operators.google_api_to_s3_transfer import GoogleApiToS3Operator
GoogleApiToS3Operator()we want to fix it as
from airflow.providers.amazon.aws.transfers.google_api_to_s3 import GoogleApiToS3Operator
GoogleApiToS3Operator()There was a problem hiding this comment.
I tried adding an unwrap call instead of ? on the get_or_import_symbol call, and I'm seeing this error when running the AIR302 tests:
called `Result::unwrap()` on an `Err` value: ConflictingName("provide_bucket_name")
It looks like you can't overwrite an existing symbol with a different import path, at least through the get_or_import_symbol API.
I don't really have a great suggestion for working around this. You could try a direct text replacement, but I can easily imagine that going wrong if there are multiple imports grouped together or something like that.
Maybe a better alternative would be a custom version of Importer::import_symbol (inside of a custom get_or_import_symbol) that ignores the ConflictingName error case since you're overwriting it anyway.
There was a problem hiding this comment.
Ah yes, I think that updating an existing import isn't currently supported and you'd need to add a dedicated edit infrastructure for it. The NumPy deprecation rules doesn't support that either so it should be fine for now unless you'd want to add support for it.
There was a problem hiding this comment.
Although this doesn't look too difficult and we could potentially use a text replacement as suggested by Brent.
There was a problem hiding this comment.
I think that's crucial for us. I'll explore a bit and see how I can achieve it. Before that, I'll apply AutoImport to fixable ones.
Summary
Add autofix to AIR303 name moved to provider
Test Plan
The existing test fixture reflects the update