Skip to content

Improve autofix to air302 #18093

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Lee-W
Copy link
Contributor

@Lee-W Lee-W commented May 14, 2025

Summary

Test Plan

let head = match_head(expr);
let binding = checker
.semantic()
.resolve_name(head.expect(""))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ntBre , I managed to put up a somewhat work version, but have a quick question. Is there a better way or correct way we can get rid of these expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it depends a bit on what could cause them to fail. I haven't really looked closely enough at match_head to see how they can fail. Just in general, you might be able to use diagnostic.try_set_optional_fix here, and you can return Ok(None) instead of calling unwrap or expect. Something like this:

let head = match_head(expr) else {
    return Ok(None);
};

That's probably the approach I would take if the result of match_head is truly optional. If it's an error you want logged, you could instead return an anyhow::Result from match_head and just use ? here.

(The main benefit of try_set_fix is that it logs any errors that occur in the closure.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see now that there are other unwrap/expect calls below. You may just want to factor out another function returning an Option<Fix> (or another type if Fix isn't right) and only call diagnostic.try_set_fix if it returns Some:

if let Some(fix) = new_fix_generating_function(/* args */) {
    diagnostic.try_set_fix(|| /* more code finalizing the fix */);
}

Then in new_fix_generating_function you can use ? anywhere you'd otherwise call unwrap or expect.

Lee-W added 3 commits May 15, 2025 18:39
… and replace them as ProviderName::AutoImport
… compatibility was not added for some cases. Thus, the following rules are moved back to AIR302

    airflow.hooks.subprocess.SubprocessResult → airflow.providers.standard.hooks.subprocess.SubprocessResult
    airflow.hooks.subprocess.working_directory → airflow.providers.standard.hooks.subprocess.working_directory
    airflow.operators.datetime.target_times_as_dates → airflow.providers.standard.operators.datetime.target_times_as_dates
    airflow.operators.trigger_dagrun.TriggerDagRunLink → airflow.providers.standard.operators.trigger_dagrun.TriggerDagRunLink
    airflow.sensors.external_task.ExternalTaskSensorLink → airflow.providers.standard.sensors.external_task.ExternalDagLink (This one contains a minor change)
    airflow.sensors.time_delta.WaitSensor → airflow.providers.standard.sensors.time_delta.WaitSensor
@Lee-W Lee-W force-pushed the improve-autofix-to-AIR302 branch from 1ba8b6b to 8860397 Compare May 15, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants