Skip to content

Conversation

@Lee-W
Copy link
Contributor

@Lee-W Lee-W commented Oct 27, 2025

…e_asset has positional arguments

Summary

  • passing positional argument into airflow.lineage.hook.HookLineageCollector.create_asset is not allowed
    • check whether positional argument with 0 index can be found

Test Plan

update the test fixture accordingly and reorganize in the next commit

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@Lee-W Lee-W force-pushed the extend-AIR301-pos-argument branch from e224675 to 4114892 Compare October 28, 2025 09:17
@Lee-W Lee-W changed the title feat(AIR301): warn if airflow.lineage.hook.HookLineageCollector.creat… [airflow] passing positional argument into airflow.lineage.hook.HookLineageCollector.create_asset is not allowed (AIR301) Oct 28, 2025
@Lee-W Lee-W marked this pull request as ready for review October 28, 2025 09:45
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I haven't followed the airflow PRs closely so I don't know if this is the first check for positional arguments.

Did airflow 2 allow position arguments but airflow 3 does no more?

I can see how this check is useful but it's something a type checker could catch too.

59 | hlc.create_asset("should", "be", "no", "posarg")
60 | hlc.create_asset(name="but", uri="kwargs are ok")
|
help: Calling ``HookLineageCollector.create_asset`` with positional argument should raise an error
Copy link
Member

Choose a reason for hiding this comment

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

Is this the intended message you want to show to users? Shouldn't it say instead that position arguments are disallowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me reword it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading the whole message yet again, we should probably add a AIR303 for function signature change. I have another local branch working on something similar and moving this one there might be better as create_asset is not removed. Only its signature changed.

WDTY?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, AIR301 feels out of place for a signature change because it's not a removal

@Lee-W
Copy link
Contributor Author

Lee-W commented Oct 29, 2025

I haven't followed the airflow PRs closely so I don't know if this is the first check for positional arguments.

Yes, I think this is the first one

Did airflow 2 allow position arguments but airflow 3 does no more?

Yes.

I can see how this check is useful but it's something a type checker could catch too.

Yes, but we think it would still be helpful if these rules can also detect it here. As most of our users would be data engineers and might not care type checking that much

apache/airflow#47615 (comment)

@Lee-W Lee-W marked this pull request as draft October 29, 2025 01:24
@MichaReiser
Copy link
Member

Yes, but we think it would still be helpful if these rules can also detect it here. As most of our users would be data engineers and might not care type checking that much

Fair enough. I'd prefer a separate rule because that would also make it easier to deprecate said rule when we introduce a general-purpose rule that detects positional arguments that should have been passed as keyword arguments (which we already have in ty)

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