potiuk commented on code in PR #27366:
URL: https://github.com/apache/airflow/pull/27366#discussion_r1008732466
##########
dev/breeze/src/airflow_breeze/utils/shared_options.py:
##########
@@ -19,7 +19,15 @@
import os
-__verbose_value: bool = os.environ.get("VERBOSE", "false")[0].lower() == "t"
+
+def __get_default_bool_value(env_var: str) -> bool:
+ string_val = os.environ.get(env_var, "")
+ if not string_val: # handle "" and other false-y coerce-able values
+ return False
+ return string_val[0].lower() in ["t", "y"] # handle all kinds of
truth-y/yes-y/false-y/non-sy strings
Review Comment:
Very good point.
Generally I want to avoid the "airflow" fallacy where there are some
intertwined imports between utils that are ending up often in circular
references and local imports (hello `config` and `secrets`) so you have to be
rather careful with such coupling of independent modules. Here the potential
difficult lies with, the Custom Params and shared_options import sequence in
various scenarios.
This would work in this case but I can imagine this migh be a problem - even
accidentally - in the future so I think even better soluion is to extract
coerce method to a separate module and use it in both places. I am rather aware
of cyclomatic complexity https://en.wikipedia.org/wiki/Cyclomatic_complexity
and while the ship has sailed for Airflow (or maybe we will be able to fix it
over time who knows I tried to fix cyclomtic complexity for config few times
and failed miserably) but I try to keep it low for breeze (even without
measuring it).
Fix is coming.
--
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]
