Repository: zulip/zulip-terminal
Pull request #1410 (2023-06-12 02:05:41)

Themes: Improve completeness checking & runtime validation
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?

This was motivated by exploring adding support back for transparency (across themes) and considering the tests and runtime checks currently in place for theme specifications.

This PR primarily involves:
- adding tests for existing theme completeness/loading behavior
- clarifying conditions for if a theme is complete
- reporting errors in theme definitions which previously resulted in tracebacks
- explicitly giving errors if required elements of a theme are absent

As a result this:
- enables safer refactoring (including in this PR)
- provides the foundation to extend theme handling (such as for transparency)
- better defines expectations of what is necessary in a theme (via tests & at runtime)
- supports simpler theme contribution

Currently:
- Incomplete themes are those lacking a `Color` enum, `STYLES` dict, and a `META` dict, with all specified sub-keys
  - note that for completeness only the keys of these are checked, not the validity of any values
- A theme lacking a `META` dict will be highlighted as incomplete, but is considered valid at runtime (syntax highlighting in a theme is optional)
  - while any arrangement to generate STYLES could be possible, `Color` is required since it is used to check for 16-color compatibility in the theme (and likely other depths later); for strict checking we should validate the colors used, not an import which we *assume* is used.

### Outstanding aspect(s)    <!-- DELETE SECTION IF EMPTY -->
<!-- In what ways is this not fully implemented/functioning? Compared to a discussion/issue? -->
<!-- Do you not understand something? Are you unsure about a certain approach? Want feedback? -->
- [ ] Documentation updates/additions based on these behaviors (followup)

### External discussion & connections
<!-- [x] all that apply, specifying topic and adding numbers after # for issues/PRs -->
- [ ] Discussed in **#zulip-terminal** in `topic`
- [ ] Fully fixes #
- [ ] Partially fixes issue #
- [ ] Builds upon previous unmerged work in PR #
- [ ] Is a follow-up to work in PR #
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #

### How did you test this?
<!-- [x] all that apply -->
- [x] Manually - Behavioral changes
- [x] Manually - Visual changes
- [x] Adapting existing automated tests
- [x] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (*only a refactor of tested code*)

### Self-review checklist for each commit
- [x] It is a [minimal coherent idea](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development)
- [x] It has a commit summary following the [documented style](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development) (title & body)
- [x] It has a commit summary describing the  motivation and reasoning for the change
- [x] It individually passes linting and tests
- [x] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit


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 e6f5f22..937f2c7 100755
--- a/zulipterminal/cli/run.py
+++ b/zulipterminal/cli/run.py
@@ -18,7 +18,7 @@ from urwid import display_common, set_encoding
 
 from zulipterminal.api_types import ServerSettings
 from zulipterminal.config.themes import (
-    InvalidThemeColorCode,
+    ThemeError,
     aliased_themes,
     all_themes,
     complete_and_incomplete_themes,
@@ -571,7 +571,7 @@ def main(options: Optional[List[str]] = None) -> None:
         zt_logger.info("\n\n%s\n\n", e)
         zt_logger.exception(e)
         exit_with_error(f"\nError connecting to Zulip server: {e}.")
-    except InvalidThemeColorCode as e:
+    except ThemeError as e:
         # Acts as separator between logs
         zt_logger.info("\n\n%s\n\n", e)
         zt_logger.exception(e)
diff --git a/zulipterminal/config/themes.py b/zulipterminal/config/themes.py
index 3d49729..c9e5174 100644
--- a/zulipterminal/config/themes.py
+++ b/zulipterminal/config/themes.py
@@ -1,7 +1,6 @@
 """
 Styles and their colour mappings in each theme, with helper functions
 """
-
 from typing import Any, Dict, List, Optional, Tuple, Union
 
 from pygments.token import STANDARD_TYPES
@@ -130,10 +129,19 @@ valid_16_color_codes = [
 ]
 
 
-class InvalidThemeColorCode(Exception):
+class ThemeError(Exception):
+    pass
+
+
+class InvalidThemeColorCode(ThemeError):
     pass
 
 
+class MissingThemeAttributeError(ThemeError):
+    def __init__(self, attribute: str) -> None:
+        super().__init__(f"Theme is missing required attribute '{attribute}'")
+
+
 def all_themes() -> List[str]:
     return list(THEMES.keys())
 
@@ -146,7 +154,10 @@ def complete_and_incomplete_themes() -> Tuple[List[str], List[str]]:
     complete = {
         name
         for name, theme in THEMES.items()
+        if getattr(theme, "Color", None)
+        if getattr(theme, "STYLES", None)
         if set(theme.STYLES) == set(REQUIRED_STYLES)
+        if getattr(theme, "META", None)
         if set(theme.META) == set(REQUIRED_META)
         for meta, conf in theme.META.items()
         if set(conf) == set(REQUIRED_META.get(meta, {}))
@@ -156,30 +167,50 @@ def complete_and_incomplete_themes() -> Tuple[List[str], List[str]]:
 
 
 def generate_theme(theme_name: str, color_depth: int) -> ThemeSpec:
-    theme_styles = THEMES[theme_name].STYLES
-    validate_colors(theme_name, color_depth)
-    urwid_theme = parse_themefile(theme_styles, color_depth)
+    theme_module = THEMES[theme_name]
 
     try:
-        theme_meta = THEMES[theme_name].META
-        add_pygments_style(theme_meta, urwid_theme)
+        theme_colors = theme_module.Color
     except AttributeError:
-        pass
+        raise MissingThemeAttributeError("Color") from None
+    validate_colors(theme_colors, color_depth)
+
+    try:
+        theme_styles = theme_module.STYLES
+    except AttributeError:
+        raise MissingThemeAttributeError("STYLES") from None
+    urwid_theme = parse_themefile(theme_styles, color_depth)
+
+    # META is not required, but if present should contain pygments data
+    theme_meta = getattr(theme_module, "META", None)
+    if theme_meta is not None:
+        pygments_data = theme_meta.get("pygments", None)
+        if pygments_data is None:
+            raise MissingThemeAttributeError('META["pygments"]') from None
+        for key in REQUIRED_META["pygments"]:
+            if pygments_data.get(key) is None:
+                raise MissingThemeAttributeError(f'META["pygments"]["{key}"]') from None
+        pygments_styles = generate_pygments_styles(pygments_data)
+    else:
+        pygments_styles = []
+
+    urwid_theme.extend(pygments_styles)
 
     return urwid_theme
 
 
-def validate_colors(theme_name: str, color_depth: int) -> None:
+# color_enum can be one of many enums satisfying the specification
+# There is currently no generic enum type
+def validate_colors(color_enum: Any, color_depth: int) -> None:
     """
     This function validates color-codes for a given theme, given colors are in `Color`.
 
     If any color is not in accordance with urwid default 16-color codes then the
     function raises InvalidThemeColorCode with the invalid colors.
     """
-    theme_colors = THEMES[theme_name].Color
     failure_text = []
     if color_depth == 16:
-        for color in theme_colors:
+        for color in color_enum:
             color_16code = color.value.split()[0]
             if color_16code not in valid_16_color_codes:
                 invalid_16_color_code = str(color.name)
@@ -188,7 +219,7 @@ def validate_colors(theme_name: str, color_depth: int) -> None:
             return
         else:
             text = "\n".join(
-                [f"Invalid 16-color codes in theme '{theme_name}':"] + failure_text
+                ["Invalid 16-color codes found in this theme:"] + failure_text
             )
             raise InvalidThemeColorCode(text)
 
@@ -224,7 +255,7 @@ def parse_themefile(
     return urwid_theme
 
 
-def add_pygments_style(theme_meta: Dict[str, Any], urwid_theme: ThemeSpec) -> None:
+def generate_pygments_styles(pygments: Dict[str, Any]) -> ThemeSpec:
     """
     This function adds pygments styles for use in syntax
     highlighting of code blocks and inline code.
@@ -239,7 +270,6 @@ def add_pygments_style(theme_meta: Dict[str, Any], urwid_theme: ThemeSpec) -> No
         used to override certain pygments styles to match to urwid format.
         It can also be used to customize the syntax style.
     """
-    pygments = theme_meta["pygments"]
     pygments_styles = pygments["styles"]
     pygments_bg = pygments["background"]
     pygments_overrides = pygments["overrides"]
@@ -247,6 +277,7 @@ def add_pygments_style(theme_meta: Dict[str, Any], urwid_theme: ThemeSpec) -> No
     term16_styles = term16.styles
     term16_bg = term16.background_color
 
+    theme_styles_from_pygments: ThemeSpec = []
     for token, css_class in STANDARD_TYPES.items():
         if css_class in pygments_overrides:
             pygments_styles[token] = pygments_overrides[css_class]
@@ -275,4 +306,5 @@ def add_pygments_style(theme_meta: Dict[str, Any], urwid_theme: ThemeSpec) -> No
             pygments_styles[token],
             pygments_bg,
         )
-        urwid_theme.append(new_style)
+        theme_styles_from_pygments.append(new_style)
+    return theme_styles_from_pygments


Test patch:
diff --git a/tests/config/test_themes.py b/tests/config/test_themes.py
index 73ae9cd..b06e35d 100644
--- a/tests/config/test_themes.py
+++ b/tests/config/test_themes.py
@@ -1,10 +1,12 @@
 import re
-from copy import deepcopy
 from enum import Enum
 from typing import Any, Dict, Optional, Tuple
 
 import pytest
+from pygments.styles.material import MaterialStyle
 from pygments.styles.perldoc import PerldocStyle
+from pygments.token import STANDARD_TYPES
+from pytest import param as case
 from pytest_mock import MockerFixture
 
 from zulipterminal.config.regexes import REGEX_COLOR_VALID_FORMATS
@@ -12,16 +14,20 @@ from zulipterminal.config.themes import (
     REQUIRED_STYLES,
     THEMES,
     InvalidThemeColorCode,
+    MissingThemeAttributeError,
     ThemeSpec,
-    add_pygments_style,
     all_themes,
     complete_and_incomplete_themes,
+    generate_pygments_styles,
+    generate_theme,
     parse_themefile,
     valid_16_color_codes,
     validate_colors,
 )
 
 
+MODULE = "zulipterminal.config.themes"
+
 expected_complete_themes = {
     "zt_dark",
     "gruvbox_dark",
@@ -86,7 +92,7 @@ def test_builtin_theme_completeness(theme_name: str) -> None:
         assert all(theme_meta[metadata][c] for c in config)
 
 
-def test_complete_and_incomplete_themes() -> None:
+def test_complete_and_incomplete_themes__bundled_theme_output() -> None:
     # These are sorted to ensure reproducibility
     result = (
         sorted(expected_complete_themes),
@@ -95,6 +101,155 @@ def test_complete_and_incomplete_themes() -> None:
     assert result == complete_and_incomplete_themes()
 
 
+@pytest.mark.parametrize(
+    "missing, expected_complete",
+    [
+        case({}, True, id="keys_complete"),
+        case({"Color": None}, False, id="Color_absent"),
+        case({"STYLES": None}, False, id="STYLES_absent"),
+        case({"STYLES": "incomplete_style"}, False, id="STYLES_incomplete"),
+        case({"META": None}, False, id="META_absent"),
+        case({"META": {}}, False, id="META_empty"),
+        case({"META": {"pygments": {}}}, False, id="META_pygments_empty"),
+    ],
+)
+def test_complete_and_incomplete_themes__single_theme_completeness(
+    mocker: MockerFixture,
+    missing: Dict[str, Any],
+    expected_complete: bool,
+    style: str = "s",
+    fake_theme_name: str = "sometheme",
+) -> None:
+    class FakeColor(Enum):
+        COLOR_1 = "a a #"
+        COLOR_2 = "k b #"
+
+    class FakeTheme:
+        Color = FakeColor
+        STYLES = {
+            style: (FakeColor.COLOR_1, FakeColor.COLOR_2) for style in REQUIRED_STYLES
+        }
+        META = {
+            "pygments": {
+                "styles": None,
+                "background": None,
+                "overrides": None,
+            }
+        }
+
+    incomplete_style = {style: (FakeColor.COLOR_1, FakeColor.COLOR_2)}
+
+    for field, action in missing.items():
+        if action == "incomplete_style":
+            setattr(FakeTheme, field, incomplete_style)
+        elif action is None:
+            delattr(FakeTheme, field)
+        else:
+            setattr(FakeTheme, field, action)
+
+    mocker.patch(MODULE + ".THEMES", {fake_theme_name: FakeTheme})
+
+    if expected_complete:
+        assert complete_and_incomplete_themes() == ([fake_theme_name], [])
+    else:
+        assert complete_and_incomplete_themes() == ([], [fake_theme_name])
+
+
+@pytest.mark.parametrize(
+    "META, expected_pygments_length",
+    [
+        case(None, 0, id="META_absent"),
+        case(
+            {
+                "pygments": {
+                    "styles": MaterialStyle().styles,
+                    "background": "h80",
+                    "overrides": {},
+                }
+            },
+            len(STANDARD_TYPES),
+            id="META_with_valid_values",
+        ),
+    ],
+)
+def test_generate_theme__has_required_attributes(
+    mocker: MockerFixture,
+    META: Optional[Dict[str, Dict[str, Any]]],
+    expected_pygments_length: int,
+    fake_theme_name: str = "fake_theme",
+    depth: int = 256,  # Only test one depth; others covered in parse_themefile tests
+    single_style: str = "somestyle",
+) -> None:
+    class FakeColor(Enum):
+        COLOR_1 = "a a #"
+        COLOR_2 = "k b #"
+
+    theme_styles = {single_style: (FakeColor.COLOR_1, FakeColor.COLOR_2)}
+
+    class FakeTheme:
+        STYLES = theme_styles
+        Color = FakeColor  # Required for validate_colors
+
+    if META is not None:
+        FakeTheme.META = META  # type: ignore [attr-defined]
+
+    mocker.patch(MODULE + ".THEMES", {fake_theme_name: FakeTheme})
+
+    generated_theme = generate_theme(fake_theme_name, depth)
+
+    assert len(generated_theme) == len(theme_styles) + expected_pygments_length
+    assert (single_style, "", "", "", "a", "b") in generated_theme
+
+
+def test_generate_theme__missing_attributes_in_theme(
+    mocker: MockerFixture,
+    fake_theme_name: str = "fake_theme",
+    depth: int = 256,
+    style: str = "somestyle",
+) -> None:
+    class FakeTheme:
+        pass
+
+    mocker.patch(MODULE + ".THEMES", {fake_theme_name: FakeTheme})
+
+    # No attributes (STYLES or META) - flag missing Color
+    with pytest.raises(MissingThemeAttributeError) as e:
+        generate_theme(fake_theme_name, depth)
+    assert str(e.value) == "Theme is missing required attribute 'Color'"
+
+    # Color but missing STYLES - flag missing STYLES
+    class FakeColor(Enum):
+        COLOR_1 = "a a #"
+        COLOR_2 = "k b #"
+
+    FakeTheme.Color = FakeColor  # type: ignore [attr-defined]
+
+    with pytest.raises(MissingThemeAttributeError) as e:
+        generate_theme(fake_theme_name, depth)
+    assert str(e.value) == "Theme is missing required attribute 'STYLES'"
+
+    # Color, STYLES and META, but no pygments data in META
+    not_all_styles = {style: (FakeColor.COLOR_1, FakeColor.COLOR_2)}
+    FakeTheme.STYLES = not_all_styles  # type: ignore [attr-defined]
+    FakeTheme.META = {}  # type: ignore [attr-defined]
+
+    with pytest.raises(MissingThemeAttributeError) as e:
+        generate_theme(fake_theme_name, depth)
+    assert str(e.value) == """Theme is missing required attribute 'META["pygments"]'"""
+
+    # Color, STYLES and META, but incomplete pygments in META
+    FakeTheme.META = {  # type: ignore [attr-defined]
+        "pygments": {"styles": "", "background": ""}
+    }
+
+    with pytest.raises(MissingThemeAttributeError) as e:
+        generate_theme(fake_theme_name, depth)
+    assert (
+        str(e.value)
+        == """Theme is missing required attribute 'META["pygments"]["overrides"]'"""
+    )
+
+
 @pytest.mark.parametrize(
     "color_depth, expected_urwid_theme",
     [
@@ -147,17 +302,15 @@ def test_parse_themefile(
 
 
 @pytest.mark.parametrize(
-    "theme_meta, expected_styles",
+    "pygments_data, expected_styles",
     [
         (
             {
-                "pygments": {
-                    "styles": PerldocStyle().styles,
-                    "background": "#def",
-                    "overrides": {
-                        "k": "#abc",
-                        "sd": "#123, bold",
-                    },
+                "styles": PerldocStyle().styles,
+                "background": "#def",
+                "overrides": {
+                    "k": "#abc",
+                    "sd": "#123, bold",
                 },
             },
             [
@@ -182,36 +335,18 @@ def test_parse_themefile(
         )
     ],
 )
-def test_add_pygments_style(
-    mocker: MockerFixture, theme_meta: Dict[str, Any], expected_styles: ThemeSpec
+def test_generate_pygments_styles(
+    mocker: MockerFixture, pygments_data: Dict[str, Any], expected_styles: ThemeSpec
 ) -> None:
-    urwid_theme: ThemeSpec = [(None, "#xxx", "#yyy")]
-    original_urwid_theme = deepcopy(urwid_theme)
-
-    add_pygments_style(theme_meta, urwid_theme)
+    pygments_styles = generate_pygments_styles(pygments_data)
 
-    # Check if original exists
-    assert original_urwid_theme[0] in urwid_theme
     # Check for overrides(k,sd) and inheriting styles (kr)
     for style in expected_styles:
-        assert style in urwid_theme
+        assert style in pygments_styles
 
 
-# Validate 16-color-codes
-@pytest.mark.parametrize(
-    "color_depth, theme_name",
-    [
-        (16, "zt_dark"),
-        (16, "gruvbox_dark"),
-        (16, "gruvbox_light"),
-        (16, "zt_light"),
-        (16, "zt_blue"),
-    ],
-)
-def test_validate_colors(theme_name: str, color_depth: int) -> None:
-    theme = THEMES[theme_name]
-
-    header_text = f"Invalid 16-color codes in theme '{theme_name}':\n"
+def test_validate_colors(color_depth: int = 16) -> None:
+    header_text = "Invalid 16-color codes found in this theme:\n"
 
     # No invalid colors
     class Color(Enum):
@@ -221,8 +356,7 @@ def test_validate_colors(theme_name: str, color_depth: int) -> None:
         GRAY_244 = "dark_gray       h244      #928374"
         LIGHT2 = "white           h250      #d5c4a1"
 
-    theme.Color = Color
-    validate_colors(theme_name, 16)
+    validate_colors(Color, color_depth)
 
     # One invalid color
     class Color1(Enum):
@@ -232,9 +366,8 @@ def test_validate_colors(theme_name: str, color_depth: int) -> None:
         GRAY_244 = "dark_gray       h244      #928374"
         LIGHT2 = "white           h250      #d5c4a1"
 
-    theme.Color = Color1
     with pytest.raises(InvalidThemeColorCode) as e:
-        validate_colors(theme_name, 16)
+        validate_colors(Color1, color_depth)
     assert str(e.value) == header_text + "- DARK0_HARD = blac"
 
     # Two invalid colors
@@ -245,9 +378,8 @@ def test_validate_colors(theme_name: str, color_depth: int) -> None:
         GRAY_244 = "dark_gra       h244      #928374"
         LIGHT2 = "white           h250      #d5c4a1"
 
-    theme.Color = Color2
     with pytest.raises(InvalidThemeColorCode) as e:
-        validate_colors(theme_name, 16)
+        validate_colors(Color2, color_depth)
     assert (
         str(e.value) == header_text + "- DARK0_HARD = blac\n" + "- GRAY_244 = dark_gra"
     )
@@ -260,9 +392,8 @@ def test_validate_colors(theme_name: str, color_depth: int) -> None:
         GRAY_244 = "dark_gra       h244      #928374"
         LIGHT2 = "whit           h250      #d5c4a1"
 
-    theme.Color = Color3
     with pytest.raises(InvalidThemeColorCode) as e:
-        validate_colors(theme_name, 16)
+        validate_colors(Color3, color_depth)
     assert (
         str(e.value)
         == header_text