Repository: zulip/zulip-terminal
Pull request #1315 (2023-03-02 23:21:58)

Refactor and simplify run.py
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->

Originally motivated by fixing a magic-number code smell highlighted by ruff, these are a sequence of refactorings which simplify run.py, the entry-point for the application. The current code works fine; this aims to make it more concise, clearer, reduce duplication and improve typing. The majority of these changes involve no changes to tests, ie. are pure refactoring.

- Enapsulate the fetching of login data in one method, without requiring the caller to know about the API/requests, including the meaning of `200` (switched to `requests.codes.OK`)
- Split validation of boolean strings (and color depth) from the translation of the boolean strings into boolean options for the controller
- Use a mapping to explicitly map from color depth strings to values passed to the controller; this also reduces duplication by using the keys for validation
- Rename NO_CONFIG to DEFAULT_CONFIG, and similarly the output string
- Extract the command-line text string into COMMANDLINE_CONFIG, much like those for ZULIPRC_CONFIG and NO_CONFIG
- Add a NamedTuple for the SettingData for each setting to improve the typing
  - This also enables another refactor to use meaningful names rather than 0/1 indexing
- Use a new print_setting helper function to simplify outputting the settings and their source before we start loading
- Migrate the *_CONFIG str constants to a ConfigSource Enum to further improve the typing
- Reorder and group independent code into new commented sections
- Slightly simplify incomplete theme logic

<!-- If fixing a filed bug or new feature, add 'Fixes #<issue>' or 'Partial fix for #<issue>' -->

<!-- Add a link to a discussion on chat.zulip.org, if relevant -->

**Tested?** <!-- Fine to leave some of these unchecked if this is a draft/work-in-progress -->
- [ ] Manually
- [x] Existing tests (adapted, if necessary)
- [ ] New tests added (for any new behavior)
- [x] Passed linting & tests (each commit)
<!-- Code must pass CI (GitHub Actions) before merging - look for the green tick! -->

**Notes & Questions** <!-- if any; add/delete/fill-in as appropriate -->

The only non-refactor here is the change from 'specified with no config' to 'specified from default config'. That can be changed easily after merging, but feedback welcome.

zulipbot: Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

<!-- areaLabelAddition -->


Patch:
diff --git a/zulipterminal/cli/run.py b/zulipterminal/cli/run.py
index 827e6d4..1f84923 100755
--- a/zulipterminal/cli/run.py
+++ b/zulipterminal/cli/run.py
@@ -9,8 +9,9 @@ import os
 import stat
 import sys
 import traceback
+from enum import Enum
 from os import path, remove
-from typing import Any, Dict, List, Optional, Tuple
+from typing import Dict, List, NamedTuple, Optional, Tuple
 
 import requests
 from urwid import display_common, set_encoding
@@ -28,10 +29,19 @@ from zulipterminal.platform_code import detected_platform
 from zulipterminal.version import ZT_VERSION
 
 
+class ConfigSource(Enum):
+    DEFAULT = "from default config"
+    ZULIPRC = "in zuliprc file"
+    COMMANDLINE = "on command line"
+
+
+class SettingData(NamedTuple):
+    value: str
+    source: ConfigSource
+
+
 TRACEBACK_LOG_FILENAME = "zulip-terminal-tracebacks.log"
 API_CALL_LOG_FILENAME = "zulip-terminal-API-requests.log"
-ZULIPRC_CONFIG = "in zuliprc file"
-NO_CONFIG = "with no config"
 
 # Create a logger for this application
 zt_logger = logging.getLogger(__name__)
@@ -46,6 +56,19 @@ zt_logger.addHandler(zt_logfile_handler)
 requests_logger = logging.getLogger("urllib3")
 requests_logger.setLevel(logging.DEBUG)
 
+# Valid boolean settings, which map from (str, str) to (True, False)
+VALID_BOOLEAN_SETTINGS: Dict[str, Tuple[str, str]] = {
+    "autohide": ("autohide", "no_autohide"),
+    "notify": ("enabled", "disabled"),
+}
+
+COLOR_DEPTH_ARGS_TO_DEPTHS: Dict[str, int] = {
+    "1": 1,
+    "16": 16,
+    "256": 256,
+    "24bit": 2**24,
+}
+
 # These should be the defaults without config file or command-line overrides
 DEFAULT_SETTINGS = {
     "theme": "zt_dark",
@@ -55,6 +78,9 @@ DEFAULT_SETTINGS = {
     "color-depth": "256",
     "maximum-footlinks": "3",
 }
+assert DEFAULT_SETTINGS["autohide"] in VALID_BOOLEAN_SETTINGS["autohide"]
+assert DEFAULT_SETTINGS["notify"] in VALID_BOOLEAN_SETTINGS["notify"]
+assert DEFAULT_SETTINGS["color-depth"] in COLOR_DEPTH_ARGS_TO_DEPTHS
 
 
 def in_color(color: str, text: str) -> str:
@@ -113,7 +139,7 @@ def parse_args(argv: List[str]) -> argparse.Namespace:
     )
     parser.add_argument(
         "--color-depth",
-        choices=["1", "16", "256", "24bit"],
+        choices=COLOR_DEPTH_ARGS_TO_DEPTHS,
         help=f"force the color depth (default: {DEFAULT_SETTINGS['color-depth']})",
     )
     parser.add_argument(
@@ -196,7 +222,7 @@ def get_login_id(realm_url: str) -> str:
     return styled_input(label)
 
 
-def get_api_key(realm_url: str) -> Tuple[requests.Response, str]:
+def get_api_key(realm_url: str) -> Optional[Tuple[str, str]]:
     from getpass import getpass
 
     login_id = get_login_id(realm_url)
@@ -208,7 +234,9 @@ def get_api_key(realm_url: str) -> Tuple[requests.Response, str]:
             "password": password,
         },
     )
-    return response, login_id
+    if response.status_code == requests.codes.OK:
+        return login_id, str(response.json()["api_key"])
+    return None
 
 
 def fetch_zuliprc(zuliprc_path: str) -> None:
@@ -231,16 +259,17 @@ def fetch_zuliprc(zuliprc_path: str) -> None:
     # Remove trailing "/"s from realm_url to simplify the below logic
     # for adding "/api"
     realm_url = realm_url.rstrip("/")
-    res, login_id = get_api_key(realm_url)
+    login_data = get_api_key(realm_url)
 
-    while res.status_code != 200:
+    while login_data is None:
         print(in_color("red", "\nIncorrect Email(or Username) or Password!\n"))
-        res, login_id = get_api_key(realm_url)
+        login_data = get_api_key(realm_url)
 
+    login_id, api_key = login_data
     save_zuliprc_failure = _write_zuliprc(
         zuliprc_path,
         login_id=login_id,
-        api_key=str(res.json()["api_key"]),
+        api_key=api_key,
         server_url=realm_url,
     )
     if not save_zuliprc_failure:
@@ -268,7 +297,7 @@ def _write_zuliprc(
         return f"{ex.__class__.__name__}: zuliprc could not be created at {to_path}"
 
 
-def parse_zuliprc(zuliprc_str: str) -> Dict[str, Any]:
+def parse_zuliprc(zuliprc_str: str) -> Dict[str, SettingData]:
     zuliprc_path = path.expanduser(zuliprc_str)
     while not path.exists(zuliprc_path):
         try:
@@ -312,13 +341,14 @@ def parse_zuliprc(zuliprc_str: str) -> Dict[str, Any]:
 
     # Initialize with default settings
     settings = {
-        setting: (default, NO_CONFIG) for setting, default in DEFAULT_SETTINGS.items()
+        setting: SettingData(default, ConfigSource.DEFAULT)
+        for setting, default in DEFAULT_SETTINGS.items()
     }
 
     if "zterm" in zuliprc:
         config = zuliprc["zterm"]
         for conf in config:
-            settings[conf] = (config[conf], ZULIPRC_CONFIG)
+            settings[conf] = SettingData(config[conf], ConfigSource.ZULIPRC)
 
     return settings
 
@@ -385,25 +415,18 @@ def main(options: Optional[List[str]] = None) -> None:
     try:
         zterm = parse_zuliprc(zuliprc_path)
 
-        if args.autohide:
-            zterm["autohide"] = (args.autohide, "on command line")
-
-        if args.theme:
-            theme_to_use = (args.theme, "on command line")
-        else:
-            theme_to_use = zterm["theme"]
-
+        ### Validate footlinks settings (not from command line)
         if (
-            zterm["footlinks"][1] == ZULIPRC_CONFIG
-            and zterm["maximum-footlinks"][1] == ZULIPRC_CONFIG
+            zterm["footlinks"].source == ConfigSource.ZULIPRC
+            and zterm["maximum-footlinks"].source == ConfigSource.ZULIPRC
         ):
             exit_with_error(
                 "Configuration Error: "
                 "footlinks and maximum-footlinks options cannot be used together"
             )
 
-        if zterm["maximum-footlinks"][1] == ZULIPRC_CONFIG:
-            maximum_footlinks = int(zterm["maximum-footlinks"][0])
+        if zterm["maximum-footlinks"].source == ConfigSource.ZULIPRC:
+            maximum_footlinks = int(zterm["maximum-footlinks"].value)
             if maximum_footlinks < 0:
                 exit_with_error(
                     "Configuration Error: "
@@ -411,85 +434,58 @@ def main(options: Optional[List[str]] = None) -> None:
                     f"you used '{maximum_footlinks}'"
                 )
 
-        if zterm["footlinks"][1] == ZULIPRC_CONFIG:
-            if zterm["footlinks"][0] == DEFAULT_SETTINGS["footlinks"]:
+        if zterm["footlinks"].source == ConfigSource.ZULIPRC:
+            if zterm["footlinks"].value == DEFAULT_SETTINGS["footlinks"]:
                 maximum_footlinks = 3
             else:
                 maximum_footlinks = 0
         else:
-            maximum_footlinks = int(zterm["maximum-footlinks"][0])
+            maximum_footlinks = int(zterm["maximum-footlinks"].value)
 
+        ### Load theme override & validate
+        if args.theme:
+            theme_to_use = SettingData(args.theme, ConfigSource.COMMANDLINE)
+        else:
+            theme_to_use = zterm["theme"]
+
+        theme_alias_suffix = ""
         available_themes = all_themes()
         theme_aliases = aliased_themes()
         is_valid_theme = (
-            theme_to_use[0] in available_themes or theme_to_use[0] in theme_aliases
+            theme_to_use.value in available_themes
+            or theme_to_use.value in theme_aliases
         )
         if not is_valid_theme:
             exit_with_error(
                 "Invalid theme '{}' was specified {}.".format(*theme_to_use),
                 helper_text=list_themes(),
             )
-        if theme_to_use[0] not in available_themes:
+        if theme_to_use.value not in available_themes:
             # theme must be an alias, as it is valid
-            real_theme_name = theme_aliases[theme_to_use[0]]
-            theme_to_use = (
-                real_theme_name,
-                f"{theme_to_use[1]} (by alias '{theme_to_use[0]}')",
-            )
+            theme_alias_suffix = f" (by alias '{theme_to_use.value}')"
+            real_theme_name = theme_aliases[theme_to_use.value]
+            theme_to_use = SettingData(real_theme_name, theme_to_use.source)
 
-        if args.color_depth:
-            zterm["color-depth"] = (args.color_depth, "on command line")
+        ### Load overrides & validate remaining settings
+        if args.autohide:
+            zterm["autohide"] = SettingData(args.autohide, ConfigSource.COMMANDLINE)
 
-        color_depth_str = zterm["color-depth"][0]
-        if color_depth_str == "24bit":
-            color_depth = 2**24
-        else:
-            color_depth = int(color_depth_str)
+        if args.color_depth:
+            zterm["color-depth"] = SettingData(
+                args.color_depth, ConfigSource.COMMANDLINE
+            )
 
         if args.notify:
-            zterm["notify"] = (args.notify, "on command line")
+            zterm["notify"] = SettingData(args.notify, ConfigSource.COMMANDLINE)
 
-        print("Loading with:")
-        print("   theme '{}' specified {}.".format(*theme_to_use))
-        complete, incomplete = complete_and_incomplete_themes()
-        if theme_to_use[0] in incomplete:
-            if complete:
-                incomplete_theme_warning = (
-                    "   WARNING: Incomplete theme; results may vary!\n"
-                    "      (you could try: {})".format(", ".join(complete))
-                )
-            else:
-                incomplete_theme_warning = (
-                    "   WARNING: Incomplete theme; results may vary!\n"
-                    "      (all themes are incomplete)"
-                )
-            print(in_color("yellow", incomplete_theme_warning))
-        print("   autohide setting '{}' specified {}.".format(*zterm["autohide"]))
-        if zterm["footlinks"][1] == ZULIPRC_CONFIG:
-            print(
-                "   maximum footlinks value '{}' specified {} from footlinks.".format(
-                    maximum_footlinks, zterm["footlinks"][1]
-                )
-            )
-        else:
-            print(
-                "   maximum footlinks value '{}' specified {}.".format(
-                    *zterm["maximum-footlinks"]
-                )
-            )
-        print("   color depth setting '{}' specified {}.".format(*zterm["color-depth"]))
-        print("   notify setting '{}' specified {}.".format(*zterm["notify"]))
-
-        # For binary settings
-        # Specify setting in order True, False
-        valid_settings = {
-            "autohide": ["autohide", "no_autohide"],
-            "notify": ["enabled", "disabled"],
-            "color-depth": ["1", "16", "256", "24bit"],
-        }
-        boolean_settings: Dict[str, bool] = dict()
-        for setting, valid_values in valid_settings.items():
-            if zterm[setting][0] not in valid_values:
+        valid_remaining_settings = dict(
+            VALID_BOOLEAN_SETTINGS,
+            **{"color-depth": COLOR_DEPTH_ARGS_TO_DEPTHS},
+        )
+
+        # Validate remaining settings
+        for setting, valid_values in valid_remaining_settings.items():
+            if zterm[setting].value not in valid_values:
                 helper_text = (
                     ["Valid values are:"]
                     + [f"  {option}" for option in valid_values]
@@ -501,16 +497,49 @@ def main(options: Optional[List[str]] = None) -> None:
                     ),
                     helper_text="\n".join(helper_text),
                 )
-            if setting == "color-depth":
-                break
-            boolean_settings[setting] = zterm[setting][0] == valid_values[0]
 
-        theme_data = generate_theme(theme_to_use[0], color_depth)
+        def print_setting(setting: str, data: SettingData, suffix: str = "") -> None:
+            print(f"   {setting} '{data.value}' specified {data.source.value}{suffix}.")
+
+        ### Let the user know we're starting to load, with what options, from where
+        print("Loading with:")
+        print_setting("theme", theme_to_use, theme_alias_suffix)
+        complete, incomplete = complete_and_incomplete_themes()
+        if theme_to_use.value in incomplete:
+            incomplete_theme_text = "   WARNING: Incomplete theme; results may vary!\n"
+            if complete:
+                incomplete_theme_text += f"      (you could try: {', '.join(complete)})"
+            else:
+                incomplete_theme_text += "      (all themes are incomplete)"
+            print(in_color("yellow", incomplete_theme_text))
+        print_setting("autohide setting", zterm["autohide"])
+        if zterm["footlinks"].source == ConfigSource.ZULIPRC:
+            print_setting(
+                "maximum footlinks value",
+                SettingData(str(maximum_footlinks), zterm["footlinks"].source),
+                " from footlinks",
+            )
+        else:
+            print_setting("maximum footlinks value", zterm["maximum-footlinks"])
+        print_setting("color depth setting", zterm["color-depth"])
+        print_setting("notify setting", zterm["notify"])
+
+        ### Generate data not output to user, but into Controller
+        # Generate urwid palette
+        color_depth_str = zterm["color-depth"].value
+        color_depth = COLOR_DEPTH_ARGS_TO_DEPTHS[color_depth_str]
+
+        theme_data = generate_theme(theme_to_use.value, color_depth)
+
+        # Translate valid strings for boolean values into True/False
+        boolean_settings: Dict[str, bool] = dict()
+        for setting, valid_values in VALID_BOOLEAN_SETTINGS.items():
+            boolean_settings[setting] = zterm[setting].value == valid_values[0]
 
         Controller(
             config_file=zuliprc_path,
             maximum_footlinks=maximum_footlinks,
-            theme_name=theme_to_use[0],
+            theme_name=theme_to_use.value,
             theme=theme_data,
             color_depth=color_depth,
             in_explore_mode=args.explore,


Test patch:
diff --git a/tests/cli/test_run.py b/tests/cli/test_run.py
index d6a896c..7653425 100644
--- a/tests/cli/test_run.py
+++ b/tests/cli/test_run.py
@@ -136,11 +136,11 @@ def test_valid_zuliprc_but_no_connection(
     expected_lines = [
         f"Detected platform: {platform}",
         "Loading with:",
-        "   theme 'zt_dark' specified with no config.",
-        "   autohide setting 'no_autohide' specified with no config.",
-        "   maximum footlinks value '3' specified with no config.",
-        "   color depth setting '256' specified with no config.",
-        "   notify setting 'disabled' specified with no config.",
+        "   theme 'zt_dark' specified from default config.",
+        "   autohide setting 'no_autohide' specified from default config.",
+        "   maximum footlinks value '3' specified from default config.",
+        "   color depth setting '256' specified from default config.",
+        "   notify setting 'disabled' specified from default config.",
         "\x1b[91m",
         f"Error connecting to Zulip server: {server_connection_error}.\x1b[0m",
     ]
@@ -192,10 +192,10 @@ def test_warning_regarding_incomplete_theme(
         f"   theme '{bad_theme}' specified on command line.",
         "\x1b[93m   WARNING: Incomplete theme; results may vary!",
         f"      {expected_warning}\x1b[0m",
-        "   autohide setting 'no_autohide' specified with no config.",
-        "   maximum footlinks value '3' specified with no config.",
-        "   color depth setting '256' specified with no config.",
-        "   notify setting 'disabled' specified with no config.",
+        "   autohide setting 'no_autohide' specified from default config.",
+        "   maximum footlinks value '3' specified from default config.",
+        "   color depth setting '256' specified from default config.",
+        "   notify setting 'disabled' specified from default config.",
         "\x1b[91m",
         f"Error connecting to Zulip server: {server_connection_error}.\x1b[0m",
     ]
@@ -328,8 +328,7 @@ def test_main_cannot_write_zuliprc_given_good_credentials(
 
     # Give some arbitrary input and fake that it's always valid
     mocker.patch.object(builtins, "input", lambda _: "text\n")
-    response = mocker.Mock(json=lambda: dict(api_key=""), status_code=200)
-    mocker.patch(MODULE + ".get_api_key", return_value=(response, None))
+    mocker.patch(MODULE + ".get_api_key", return_value=("my_login", "my_api_key"))
 
     with pytest.raises(SystemExit):
         main([])