dstandish commented on code in PR #44226:
URL: https://github.com/apache/airflow/pull/44226#discussion_r1852130122
##########
airflow/api_fastapi/core_api/routes/public/dags.py:
##########
@@ -126,8 +128,10 @@ def get_dag_tags(
limit=limit,
session=session,
)
+ if TYPE_CHECKING:
+ assert total_entries is not None
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)
Review 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 🤔
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
