You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "rawwar (via GitHub)" <gi...@apache.org> on 2024/11/25 07:18:30 UTC
[PR] [Refactor] Move orderby type annotations to parameters [airflow]
rawwar opened a new pull request, #44335:
URL: https://github.com/apache/airflow/pull/44335
Move orderby type annotations to parameters
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Refactor] Move orderby type annotations of Assets endpoint to parameters [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #44335:
URL: https://github.com/apache/airflow/pull/44335#issuecomment-2498404049
> You are correct. I just checked. How about declaring sortable columns as constants and probably update SortParams to get these based on the Model?
I don't know, as long as these hardcoded array of string (sortable columns) are specified once, I don't feel the need to over factorize or simplify.
We cannot put the `sortable_columns` on the Model. Otherwise it introduce a dependency from the db layer to the webserver views/routes. That cannot be possible, the dependency should be the other way around, so the mapping between Models and sortable column has to leave somewhere else, probably in the webserver (FastAPI API). Then we end up with a mapping of constant arrays and SQLAlchemy ... and I find this quite messy.
But maybe you have an idea on how to achieve a cleaner approach, I'd be glad to review a PR for that.
Maybe another soution is instead of having 'strings' those could already be references to the actual column `DagRun.start_date`, etc... etc... benefiting from code completion, references, and removing hardcoded strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Refactor] Move orderby type annotations of Assets endpoint to parameters [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #44335:
URL: https://github.com/apache/airflow/pull/44335#issuecomment-2498028871
> Why do we only move the `order_by` from assets into the `parameters` and not the others ?
I did not want to do everything before confirming if this is a good idea. I'll update this PR to include all endpoints
> The idea of `parameters.py` is for re-usable parameters, but those seem to be very specific to 1 route.
>
> Do we plan on re-using them ?
Honestly, we might have to, if we plan to add more endpoints. It is similar to DagWarning filter or others. Not reusable right now. But, can be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Refactor] Move orderby type annotations of Assets endpoint to parameters [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #44335:
URL: https://github.com/apache/airflow/pull/44335#issuecomment-2498122691
> I don't think they will be reused actually. OrderBy are specific for each resource (dagrun, dag, asset, pool, variables, etc...).
>
> And for 1 specific resource, the only endpoint that leverage this ordering is the `get_resources` (listing the resources).
>
> Do we have any `order_by` that is re-used by at least 2 endpoints ?
You are correct. I just checked. How about declaring sortable columns as constants and probably using a function to generate the Annotation? If this does not sound good, I'll just close this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Refactor] Move orderby type annotations of Assets endpoint to parameters [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #44335: [Refactor] Move orderby type annotations of Assets endpoint to parameters
URL: https://github.com/apache/airflow/pull/44335
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Refactor] Move orderby type annotations of Assets endpoint to parameters [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44335:
URL: https://github.com/apache/airflow/pull/44335#issuecomment-2584944467
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Refactor] Move orderby type annotations of Assets endpoint to parameters [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #44335:
URL: https://github.com/apache/airflow/pull/44335#issuecomment-2498056398
I don't think they will be reused actually. OrderBy are specific for each resource (dagrun, dag, asset, pool, variables, etc...).
And for 1 specific resource, the only endpoint that leverage this ordering is the `get_resources` (listing the resources).
Do we have any order_by that are re-used at the moment by at least 2 endpoints ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org