[airflow] Passing positional argument into airflow.lineage.hook.HookLineageCollector.create_asset is not allowed (AIR303)

[sjyangkevin]

Summary

This is a follow up PR to #21096

The new code AIR303 is added for checking function signature change in Airflow 3.0. The new rule added to AIR303 will check if positional argument is passed into airflow.lineage.hook.HookLineageCollector.create_asset. Since this method is updated to accept only keywords argument, passing positional argument into it is not allowed, and will raise an error. The test is done by checking whether positional argument with 0 index can be found.

Test Plan

A new test file is added to the fixtures for the code AIR303. Snapshot test is updated accordingly.

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[sjyangkevin]

Hi @Lee-W , @ntBre , I've made the following updates to the PR. The updates might be more than what we've discussed, but please let me know if you have further feedback. Thanks for your time to review.

Updates

• created a parent function airflow_3_function_signature_change_expr to call airflow_3_keyword_args_only_function
• updated the violation_metadata to 0.14.11
• updated the struct document to be more generic for function signature change
• updated some namings and description for functions
• removed the check for create_dataset, which is checked by the AIR301 for deprecation, implement only function signature change check for create_asset in AIR303 (crates/ruff_linter/src/rules/airflow/rules/function_signature_change_in_3.rs L107)
• fixed the document generation issue, CI error for document generation should be fixed

I'm not sure whether we want to make this a replacement. Or should we just make it a Message? Or this probably should be something like make pos arg passed through keyword, but we'll need to store the order of that function as well. it's not super hard but not sure whether fixing that worth the effort

I think again based on the following trade-off, I feel make it a Message probably a better option. @Lee-W let me know if you have different thoughts, or you think there is a better way to implement it.

Option 1: make it a message

• Implementation is straight-forward, and less maintenance efforts
• User experience might not be great, as they need to manually fix this, even though the fix is straight-forward
• Since it doesn't suggest any fix, no edge case to handle when suggesting/executing the fix

Option 2: make a "full replacement"

• In this case, we will need to store the order of the arguments for a function. Then every update (e.g., create_asset(uri, extra) to create_asset(uri, metadata)) would require an update to the rule. I am wondering if it could introduce long-term maintenance effort to ensure the rule work properly
• Wondering if the handling of starred args would be complicated (e.g., collector.create_asset(*some_list, **some_dict)). This might be an edge case. In node.rs, seems like there is a function can resolve the order for this case, so it could possibly be handled as well.

[Lee-W]

Left a few comments, but looks way better! Thanks!

[ntBre]

Thank you! This looks great to me overall, I just had a few small suggestions.

[sjyangkevin]

Thanks @ntBre, I've made the following changes based on the feedback.

1. Renamed the test file from AIR303_args.py to AIR303.py
2. Renamed airflow_3_function_signature_change_expr to airflow_3_function_signature_change

nit: I think I would probably just inline these functions:

3. Refactored the implementation to reduce the hierarchy of the function calls
4. Emitted the diagnostic directly to avoid the trail of else { return; } arms
5. Moved the enum FunctionSignatureChangeType into function_signature_change_in_3.rs, as it is less likely to be an enum shared by other modules/implementations. In case in future we need to tailor the message for different types of function signature changes, probably could still keep it.
6. Renamed qualname to qualified_name to avoid abbreviating names
7. Renamed Airflow3FunctionSignatureChange to Airflow3IncompatibleFunctionSignature to better align with naming convention
8. Updated the struct document for Airflow3IncompatibleFunctionSignature according to the feedback
9. Enhanced the check logic (use this condition arguments.find_positional(0).is_some() || arguments.args.iter().any(Expr::is_starred_expr);) to handle starred expressions (i.e., *args and **kwargs) ; the updated test cases and results are shown in AIR303.py and ruff_linter__rules__airflow__tests__AIR303_AIR303.py.snap.
10. Refactored the code to use if let instead of match as we only have a single pattern matching here. Using match raise a clippy warnings. We can refactor this to use match when more cases coming in the future.

if let ["airflow", "lineage", "hook", "HookLineageCollector"] = qualified_name.segments() {
    if attr.as_str() == "create_asset" && has_positional_args {
        ...

11. Update the message to show better description in the document.

@Lee-W , would you mind have a look into these changes whenever you have time, and let me know if you have further feedback?

Thanks again for both your time in reviewing this!

[ntBre]

Thank you, this looks great!

I just had a couple more nits after we mark the rule as preview. Otherwise this is good to go if @Lee-W is happy with it too :)

[Lee-W]

will take another look early tomorrow. Thanks!

[sjyangkevin]

Thanks @ntBre , I've made the following updates based on your feedback.

1. Put the enum FunctionSignatureChangeType to the bottom of the file
2. Updated stable_since to preview_since
3. Renamed airflow_3_function_signature_change to airflow_3_incompatible_function_signature to match the new Violation name
4. Refactored the logic for resolving the qualified name first resolve by assignment (the common pattern)
5. call_expr is only used to access func, directly destructure it is a better approach. Thanks!

[Lee-W]

LGTM! Thanks @sjyangkevin and @ntBre !

[ntBre]

Thank you!