Make filters param optional and fix typing #44226
Conversation
dstandish
requested review from
ephraimbuddy,
pierrejeambrun,
ashb and
XD-DENG
as code owners
boring-cyborg
bot
added
the
area:Scheduler
Given that sometimes we don't want to apply any filters, it makes sense to make the param optional. I also fix the typing on `paginated_select`.
dstandish
force-pushed
the
make-filters-optional
branch
from
2b3838f
to
697edf0
Compare
| dag_tags = session.execute(dag_tags_select).scalars().all() | ||
| return DAGTagCollectionResponse(tags=[dag_tag for dag_tag in dag_tags], total_entries=total_entries) | ||
| return DAGTagCollectionResponse(tags=[x for x in dag_tags], total_entries=total_entries) |
There was a problem hiding this comment.
not sure why we change this 🤔 the original one looks better to me
There was a problem hiding this comment.
yeah i figured this might raise an eyebrow
but here's why.
because dag_tags looks so much like dag_tag, i think the readability of this is bad.
it also commonly makes it spil over many lines, which while not necessarily a bad thing, it can make comprehensions harder to read.
also, with a one-variable comprehension, there's really no need to give the variable a meaningful name. it's obvious.
having good variable names is more important when the variable will have to be read out of the context. or in a comprehension when you have multiple vars. here it's doing more harm than good.
but now that we are looking at it, i actually am not sure it even needs to be a comprehension 🤔
| @@ -274,7 +281,7 @@ def patch_dags( | |||
| ) | |||
|
|
|||
| return DAGCollectionResponse( | |||
| dags=[DAGResponse.model_validate(dag, from_attributes=True) for dag in dags], | |||
| dags=[DAGResponse.model_validate(x, from_attributes=True) for x in dags], | |||
| @@ -126,21 +126,17 @@ def get_event_logs( | |||
| base_select = base_select.where(Log.dttm > after) | |||
| event_logs_select, total_entries = paginated_select( | |||
| base_select, | |||
| [], | |||
| None, | |||
There was a problem hiding this comment.
We probably could use keyword argment here like the change in airflow/api_fastapi/core_api/routes/public/connections.py?
There was a problem hiding this comment.
yeah i think kwargs only could be a followup. but i did not want to take that on here
| if TYPE_CHECKING: | ||
| assert isinstance(total_entries, int) |
There was a problem hiding this comment.
What is the purpose of these code blocks ?
As I understand, at type checking time, the function get_query_count is typed to return int. (static tool should not have any trouble with that).
At run time this is ignored anyway.
I might be missing something.
There was a problem hiding this comment.
if you look at the signature of paginated_select, note that i fixed it -- it used to say -> Select but now i say -> tuple[Select, int | None]. This is why the typing shenanigans are required. If we remove possible optionality of the count, then this would not be necessary.
I think the count optionality was added recently.
There was a problem hiding this comment.
Cf ash comment about overload, I was just about to write that and figured ash beat me to it.
| if TYPE_CHECKING: | ||
| assert isinstance(total_entries, int) |
There was a problem hiding this comment.
Using an @overload on paginated_select would fix this:
@overload
def paginated_select(
base_select: Select,
filters: Sequence[BaseParam] | None = None,
order_by: BaseParam | None = None,
offset: BaseParam | None = None,
limit: BaseParam | None = None,
session: Session = NEW_SESSION,
return_total_entries: Literal[True] = True,
) -> tuple[Select, int]: ...
@overload
def paginated_select(
base_select: Select,
filters: Sequence[BaseParam] | None = None,
order_by: BaseParam | None = None,
offset: BaseParam | None = None,
limit: BaseParam | None = None,
session: Session = NEW_SESSION,
return_total_entries: Literal[False],
) -> tuple[Select, None]: ...
@provide_session
def paginated_select(
base_select: Select,
filters: Sequence[BaseParam] | None = None,
order_by: BaseParam | None = None,
offset: BaseParam | None = None,
limit: BaseParam | None = None,
session: Session = NEW_SESSION,
return_total_entries: bool = True,
) -> tuple[Select, int | None]:This tells typecheckers "if you pass True for return_total_entries you get an int, else you get None.
There was a problem hiding this comment.
nice, that's good solution
| ) -> Select: | ||
| if filters is None: | ||
| return base_select | ||
| for f in filters: |
There was a problem hiding this comment.
| for f in filters: | |
| for filter in filters: |
There was a problem hiding this comment.
I thought of it. but filter is a keyword. probably not the best idea to do so 🤔
There was a problem hiding this comment.
right that's why the rename here, since filter is keyword.
also again, the 'off-by-s' thing, it's not great for readability.
| if filters is None: | ||
| return base_select | ||
| for f in filters: | ||
| if f is None: |
There was a problem hiding this comment.
| if f is None: | |
| if filter is None: |
| continue | ||
| base_select = filter.to_orm(base_select) | ||
| base_select = f.to_orm(base_select) |
There was a problem hiding this comment.
| base_select = f.to_orm(base_select) | |
| base_select = filter.to_orm(base_select) |
Given that sometimes we don't want to apply any filters, it makes sense to make the param optional. I also fix the typing on
paginated_select.