[GitHub] [airflow] hankehly commented on pull request #24770: Only refresh active dags on dags page

[GitBox <gi...@apache.org>]

hankehly commented on PR #24770:
URL: https://github.com/apache/airflow/pull/24770#issuecomment-1179475253

   @bbovenzi Thanks for the review!
   
   I made changes based on your feedback.
   
   > I'm not a big fan of passing functions and then reassigning those arguments in refreshDagStats() and handleRefresh(). It's a lot of extra logic to follow. I think it would be clearer to pass a refreshAll boolean variable, and even combine getDagIds and getAllDagIds into a single function.
   
   Thanks for your insight. While I think passing functions can make the code more extensible/tolerant to change (kind of like the strategy pattern in OOP) it definitely isn't required to complete this task.
   
   I do think passing functions has benefits (like the following), so I hope we can consider this approach in the future, maybe in a different context.
   
   benefits in this case:
   - by passing in a different function to `handleRefresh`, we can modify its behavior without changing its implementation/tests
   - writing out something like `handleRefresh(getAllDagIds)` gives a little more information about which dags are being targeted as opposed to `handleRefresh(true)` (if only we could write `handleRefresh(activeOnly=true)`)
   
   Thanks again.


-- 
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