You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/01 20:36:02 UTC

[GitHub] [airflow] blag commented on a diff in pull request #25449: Move breeze commands to sub-commands

blag commented on code in PR #25449:
URL: https://github.com/apache/airflow/pull/25449#discussion_r934863980


##########
dev/breeze/src/airflow_breeze/configure_rich_click.py:
##########
@@ -20,26 +20,27 @@
     # We handle ImportError so that click autocomplete works
     import rich_click as click
 
-    from airflow_breeze.commands.ci_commands import CI_COMMANDS, CI_PARAMETERS
-    from airflow_breeze.commands.ci_image_commands import CI_IMAGE_TOOLS_COMMANDS, CI_IMAGE_TOOLS_PARAMETERS
-    from airflow_breeze.commands.configuration_and_maintenance_commands import (
-        CONFIGURATION_AND_MAINTENANCE_COMMANDS,
-        CONFIGURATION_AND_MAINTENANCE_PARAMETERS,
+    from airflow_breeze.commands.ci_commands_config import CI_COMMANDS, CI_PARAMETERS

Review Comment:
   Suggestion: move the `except` block up here and throw the rest of the `try` block (eg: everything after the `import rich_click as click` line) into an `else` block:
   
   ```suggestion
   except ImportError as e:
       if "No module named 'rich_click'" in e.msg:
           # just ignore the import error when rich_click is missing
           import click  # type: ignore[no-redef]
       else:
           raise
   else:
       from airflow_breeze.commands.ci_commands_config import CI_COMMANDS, CI_PARAMETERS
   ```



##########
dev/breeze/src/airflow_breeze/utils/console.py:
##########
@@ -79,3 +79,15 @@ def get_console() -> Console:
         theme=get_theme(),
         record=True if recording_file else False,
     )
+
+
+@lru_cache(maxsize=None)
+def get_stderr_console() -> Console:
+    return Console(
+        force_terminal=True,
+        color_system="standard",
+        stderr=True,
+        width=180 if not recording_width else int(recording_width),

Review Comment:
   This is probably a larger discussion (so feel free to ignore), but doesn't it make sense to set this number to match the [flake8 max line length](https://github.com/apache/airflow/blob/c5878315f3df0854a0902d91668cc369e7466405/.flake8#L2), the [editor configuration](https://github.com/apache/airflow/blob/c5878315f3df0854a0902d91668cc369e7466405/.editorconfig#L29), the [`isort` line length](https://github.com/apache/airflow/blob/c5878315f3df0854a0902d91668cc369e7466405/setup.cfg#L208), and the [`black` line length limit](https://github.com/apache/airflow/blob/c5878315f3df0854a0902d91668cc369e7466405/pyproject.toml#L18) of 110?



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