Repository: Textualize/textual
Pull request: Splitting out parsing of durations into new token types, avoiding Scalar

Problem statement:
Extract time values from CSS
Values for time in transition rule (and potentially elsewhere) are parsed in to a Scalar object.

Time is technically a scalar, but it adds a bit of confusion to the Scalar class which is mostly used for dimensions.

We should drop the use of Scalar for time and define another object to store the times. Perhaps an object for time is overkill, and we should simply use a float for seconds.

Ask WM for a tour of how CSS is parsed in Textual before attempting this one.

Hints from issue discussion:

Commit message: Ensure test accounts for negative values

Patch to review:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index e2c607c2f5..e26fb347e6 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -11,3 +11,4 @@ repos:
     rev: 21.8b0
     hooks:
     -   id: black
+        exclude: ^tests/
diff --git a/Makefile b/Makefile
index 80cde06d37..16489186e4 100644
--- a/Makefile
+++ b/Makefile
@@ -5,4 +5,4 @@ typecheck:
 format:
 	black src
 format-check:
-	black --check .
+	black --check src
diff --git a/pyproject.toml b/pyproject.toml
index 0572f12afa..fdf571d96e 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -17,7 +17,6 @@ classifiers = [
     "Programming Language :: Python :: 3.10",
 ]
 
-
 [tool.poetry.dependencies]
 python = "^3.7"
 rich = "^10.12.0"
@@ -25,7 +24,6 @@ rich = "^10.12.0"
 typing-extensions = { version = "^3.10.0", python = "<3.8" }
 
 [tool.poetry.dev-dependencies]
-
 pytest = "^6.2.3"
 black = "^21.11b1"
 mypy = "^0.910"
@@ -35,6 +33,9 @@ mkdocstrings = "^0.15.2"
 mkdocs-material = "^7.1.10"
 pre-commit = "^2.13.0"
 
+[tool.black]
+includes = "src"
+
 [build-system]
 requires = ["poetry-core>=1.0.0"]
 build-backend = "poetry.core.masonry.api"
diff --git a/src/textual/_duration.py b/src/textual/_duration.py
new file mode 100644
index 0000000000..04aeace07e
--- /dev/null
+++ b/src/textual/_duration.py
@@ -0,0 +1,43 @@
+import re
+
+_match_duration = re.compile(r"^(-?\d+\.?\d*)(s|ms)$").match
+
+
+class DurationError(Exception):
+    pass
+
+
+class DurationParseError(DurationError):
+    pass
+
+
+def _duration_as_seconds(duration: str) -> float:
+    """
+    Args:
+        duration (str): A string of the form ``"2s"`` or ``"300ms"``, representing 2 seconds and
+            300 milliseconds respectively. If no unit is supplied, e.g. ``"2"``, then the duration is
+            assumed to be in seconds.
+    Raises:
+        DurationParseError: If the argument ``duration`` is not a valid duration string.
+    Returns:
+        float: The duration in seconds.
+
+    """
+    match = _match_duration(duration)
+
+    if match:
+        value, unit_name = match.groups()
+        value = float(value)
+        if unit_name == "ms":
+            duration_secs = value / 1000
+        else:
+            duration_secs = value
+    else:
+        try:
+            duration_secs = float(duration)
+        except ValueError:
+            raise DurationParseError(
+                f"'{duration}' is not a valid duration."
+            ) from ValueError
+
+    return duration_secs
diff --git a/src/textual/css/_style_properties.py b/src/textual/css/_style_properties.py
index a1c42d2dd8..3ed32ee8be 100644
--- a/src/textual/css/_style_properties.py
+++ b/src/textual/css/_style_properties.py
@@ -55,7 +55,6 @@ def __get__(
     def __set__(
         self, obj: Styles, value: float | Scalar | str | None
     ) -> float | Scalar | str | None:
-        new_value: Scalar | None = None
         if value is None:
             new_value = None
         elif isinstance(value, float):
diff --git a/src/textual/css/_styles_builder.py b/src/textual/css/_styles_builder.py
index 560adf323b..17349a04f7 100644
--- a/src/textual/css/_styles_builder.py
+++ b/src/textual/css/_styles_builder.py
@@ -1,11 +1,3 @@
-"""
-
-The StylesBuilder object takes tokens parsed from the CSS and converts
-to the appropriate internal types.
-
-
-"""
-
 from __future__ import annotations
 
 from typing import cast, Iterable, NoReturn
@@ -17,6 +9,7 @@
 from .constants import VALID_BORDER, VALID_EDGE, VALID_DISPLAY, VALID_VISIBILITY
 from .errors import DeclarationError
 from ._error_tools import friendly_list
+from .._duration import _duration_as_seconds
 from .._easing import EASING
 from ..geometry import Spacing, SpacingDimensions
 from .model import Declaration
@@ -28,6 +21,11 @@
 
 
 class StylesBuilder:
+    """
+    The StylesBuilder object takes tokens parsed from the CSS and converts
+    to the appropriate internal types.
+    """
+
     def __init__(self) -> None:
         self.styles = Styles()
 
@@ -240,14 +238,21 @@ def process_offset(self, name: str, tokens: list[Token], important: bool) -> Non
         if not tokens:
             return
         if len(tokens) != 2:
-            self.error(name, tokens[0], "expected two numbers in declaration")
+            self.error(
+                name, tokens[0], "expected two scalars or numbers in declaration"
+            )
         else:
             token1, token2 = tokens
 
-            if token1.name != "scalar":
-                self.error(name, token1, f"expected a scalar; found {token1.value!r}")
-            if token2.name != "scalar":
-                self.error(name, token2, f"expected a scalar; found {token1.value!r}")
+            if token1.name not in ("scalar", "number"):
+                self.error(
+                    name, token1, f"expected a scalar or number; found {token1.value!r}"
+                )
+            if token2.name not in ("scalar", "number"):
+                self.error(
+                    name, token2, f"expected a scalar or number; found {token2.value!r}"
+                )
+
             scalar_x = Scalar.parse(token1.value, Unit.WIDTH)
             scalar_y = Scalar.parse(token2.value, Unit.HEIGHT)
             self.styles._rule_offset = ScalarOffset(scalar_x, scalar_y)
@@ -259,7 +264,7 @@ def process_offset_x(self, name: str, tokens: list[Token], important: bool) -> N
             self.error(name, tokens[0], f"expected a single number")
         else:
             token = tokens[0]
-            if token.name != "scalar":
+            if token.name not in ("scalar", "number"):
                 self.error(name, token, f"expected a scalar; found {token.value!r}")
             x = Scalar.parse(token.value, Unit.WIDTH)
             y = self.styles.offset.y
@@ -272,7 +277,7 @@ def process_offset_y(self, name: str, tokens: list[Token], important: bool) -> N
             self.error(name, tokens[0], f"expected a single number")
         else:
             token = tokens[0]
-            if token.name != "scalar":
+            if token.name not in ("scalar", "number"):
                 self.error(name, token, f"expected a scalar; found {token.value!r}")
             y = Scalar.parse(token.value, Unit.HEIGHT)
             x = self.styles.offset.x
@@ -394,15 +399,8 @@ def process_transition(
     ) -> None:
         transitions: dict[str, Transition] = {}
 
-        css_property = ""
-        duration = 1.0
-        easing = "linear"
-        delay = 0.0
-
-        iter_tokens = iter(tokens)
-
         def make_groups() -> Iterable[list[Token]]:
-            """Batch tokens in to comma-separated groups."""
+            """Batch tokens into comma-separated groups."""
             group: list[Token] = []
             for token in tokens:
                 if token.name == "comma":
@@ -414,6 +412,7 @@ def make_groups() -> Iterable[list[Token]]:
             if group:
                 yield group
 
+        valid_duration_token_names = ("duration", "number")
         for tokens in make_groups():
             css_property = ""
             duration = 1.0
@@ -425,13 +424,13 @@ def make_groups() -> Iterable[list[Token]]:
                 token = next(iter_tokens)
                 if token.name != "token":
                     self.error(name, token, "expected property")
-                css_property = token.value
 
+                css_property = token.value
                 token = next(iter_tokens)
-                if token.name != "scalar":
-                    self.error(name, token, "expected time")
+                if token.name not in valid_duration_token_names:
+                    self.error(name, token, "expected duration or number")
                 try:
-                    duration = Scalar.parse(token.value).resolve_time()
+                    duration = _duration_as_seconds(token.value)
                 except ScalarError as error:
                     self.error(name, token, str(error))
 
@@ -448,10 +447,10 @@ def make_groups() -> Iterable[list[Token]]:
                 easing = token.value
 
                 token = next(iter_tokens)
-                if token.name != "scalar":
-                    self.error(name, token, "expected time")
+                if token.name not in valid_duration_token_names:
+                    self.error(name, token, "expected duration or number")
                 try:
-                    delay = Scalar.parse(token.value).resolve_time()
+                    delay = _duration_as_seconds(token.value)
                 except ScalarError as error:
                     self.error(name, token, str(error))
             except StopIteration:
diff --git a/src/textual/css/parse.py b/src/textual/css/parse.py
index ba0bc67832..283edd6c04 100644
--- a/src/textual/css/parse.py
+++ b/src/textual/css/parse.py
@@ -5,11 +5,11 @@
 from functools import lru_cache
 from typing import Iterator, Iterable
 
-from .styles import Styles
-from .tokenize import tokenize, tokenize_declarations, Token
-from .tokenizer import EOFError
+from textual.css.styles import Styles
+from textual.css.tokenize import tokenize, tokenize_declarations, Token
+from textual.css.tokenizer import EOFError
 
-from .model import (
+from textual.css.model import (
     Declaration,
     RuleSet,
     Selector,
@@ -17,7 +17,7 @@
     SelectorSet,
     SelectorType,
 )
-from ._styles_builder import StylesBuilder, DeclarationError
+from textual.css._styles_builder import StylesBuilder, DeclarationError
 
 
 SELECTOR_MAP: dict[str, tuple[SelectorType, tuple[int, int, int]]] = {
@@ -82,8 +82,6 @@ def parse_selectors(css_selectors: str) -> tuple[SelectorSet, ...]:
 
 def parse_rule_set(tokens: Iterator[Token], token: Token) -> Iterable[RuleSet]:
 
-    rule_set = RuleSet()
-
     get_selector = SELECTOR_MAP.get
     combinator: CombinatorType | None = CombinatorType.DESCENDENT
     selectors: list[Selector] = []
@@ -187,8 +185,8 @@ def parse_declarations(css: str, path: str) -> Styles:
                 try:
                     styles_builder.add_declaration(declaration)
                 except DeclarationError as error:
-                    raise
                     errors.append((error.token, error.message))
+                    raise
             declaration = Declaration(token, "")
             declaration.name = token.value.rstrip(":")
         elif token_name == "declaration_set_end":
@@ -201,8 +199,8 @@ def parse_declarations(css: str, path: str) -> Styles:
         try:
             styles_builder.add_declaration(declaration)
         except DeclarationError as error:
-            raise
             errors.append((error.token, error.message))
+            raise
 
     return styles_builder.styles
 
@@ -257,9 +255,21 @@ def parse(css: str, path: str) -> Iterable[RuleSet]:
 if __name__ == "__main__":
     print(parse_selectors("Foo > Bar.baz { foo: bar"))
 
-    CSS = """
-text: on red;
-docksX: main=top;
-    """
-
-    print(parse_declarations(CSS, "foo"))
+    css = """#something {
+    text: on red;
+    transition: offset 5.51s in_out_cubic;
+    offset-x: 100%;
+}
+"""
+
+    from textual.css.stylesheet import Stylesheet, StylesheetParseError
+    from rich.console import Console
+
+    console = Console()
+    stylesheet = Stylesheet()
+    try:
+        stylesheet.parse(css)
+    except StylesheetParseError as e:
+        console.print(e.errors)
+    print(stylesheet)
+    print(stylesheet.css)
diff --git a/src/textual/css/scalar.py b/src/textual/css/scalar.py
index 0f3c792e6e..eb5a52acc3 100644
--- a/src/textual/css/scalar.py
+++ b/src/textual/css/scalar.py
@@ -6,6 +6,7 @@
 
 import rich.repr
 
+from textual.css.tokenizer import Token
 from ..geometry import Offset
 
 
@@ -30,8 +31,6 @@ class Unit(Enum):
     HEIGHT = 5
     VIEW_WIDTH = 6
     VIEW_HEIGHT = 7
-    MILLISECONDS = 8
-    SECONDS = 9
 
 
 UNIT_SYMBOL = {
@@ -42,13 +41,11 @@ class Unit(Enum):
     Unit.HEIGHT: "h",
     Unit.VIEW_WIDTH: "vw",
     Unit.VIEW_HEIGHT: "vh",
-    Unit.MILLISECONDS: "ms",
-    Unit.SECONDS: "s",
 }
 
 SYMBOL_UNIT = {v: k for k, v in UNIT_SYMBOL.items()}
 
-_MATCH_SCALAR = re.compile(r"^(\-?\d+\.?\d*)(fr|%|w|h|vw|vh|s|ms)?$").match
+_MATCH_SCALAR = re.compile(r"^(-?\d+\.?\d*)(fr|%|w|h|vw|vh)?$").match
 
 
 RESOLVE_MAP = {
@@ -142,14 +139,6 @@ def resolve_dimension(
         except KeyError:
             raise ScalarResolveError(f"expected dimensions; found {str(self)!r}")
 
-    def resolve_time(self) -> float:
-        value, unit, _ = self
-        if unit == Unit.MILLISECONDS:
-            return value / 1000.0
-        elif unit == Unit.SECONDS:
-            return value
-        raise ScalarResolveError(f"expected time; found {str(self)!r}")
-
 
 @rich.repr.auto(angular=True)
 class ScalarOffset(NamedTuple):
diff --git a/src/textual/css/stylesheet.py b/src/textual/css/stylesheet.py
index 77e4ab28ac..34a404ffa4 100644
--- a/src/textual/css/stylesheet.py
+++ b/src/textual/css/stylesheet.py
@@ -86,18 +86,18 @@ def read(self, filename: str) -> None:
                 css = css_file.read()
             path = os.path.abspath(filename)
         except Exception as error:
-            raise StylesheetError(f"unable to read {filename!r}; {error}") from None
+            raise StylesheetError(f"unable to read {filename!r};") from error
         try:
             rules = list(parse(css, path))
         except Exception as error:
-            raise StylesheetError(f"failed to parse {filename!r}; {error}") from None
+            raise StylesheetError(f"failed to parse {filename!r};") from error
         self.rules.extend(rules)
 
     def parse(self, css: str, *, path: str = "") -> None:
         try:
             rules = list(parse(css, path))
         except Exception as error:
-            raise StylesheetError(f"failed to parse css; {error}")
+            raise StylesheetError(f"failed to parse css;") from error
         self.rules.extend(rules)
         if self.any_errors:
             raise StylesheetParseError(self.error_renderable)
diff --git a/src/textual/css/tokenize.py b/src/textual/css/tokenize.py
index 80e57f5d21..2aae666e1a 100644
--- a/src/textual/css/tokenize.py
+++ b/src/textual/css/tokenize.py
@@ -1,10 +1,10 @@
 from __future__ import annotations
+
+import pprint
 import re
 from typing import Iterable
 
-from rich import print
-
-from .tokenizer import Expect, Tokenizer, Token
+from textual.css.tokenizer import Expect, Tokenizer, Token
 
 
 expect_selector = Expect(
@@ -51,7 +51,9 @@
     declaration_end=r"\n|;",
     whitespace=r"\s+",
     comment_start=r"\/\*",
-    scalar=r"\-?\d+\.?\d*(?:fr|%|w|h|vw|vh|s|ms)?",
+    scalar=r"\-?\d+\.?\d*(?:fr|%|w|h|vw|vh)",
+    duration=r"\d+\.?\d*(?:ms|s)",
+    number=r"\-?\d+\.?\d*",
     color=r"\#[0-9a-fA-F]{6}|color\([0-9]{1,3}\)|rgb\(\d{1,3}\,\s?\d{1,3}\,\s?\d{1,3}\)",
     key_value=r"[a-zA-Z_-][a-zA-Z0-9_-]*=[0-9a-zA-Z_\-\/]+",
     token="[a-zA-Z_-]+",
@@ -124,3 +126,13 @@ class DeclarationTokenizerState(TokenizerState):
 #             break
 #         expect = get_state(name, expect)
 #         yield token
+
+if __name__ == "__main__":
+    css = """#something {
+        text: on red;
+        offset-x: 10;
+    }
+    """
+    # transition: offset 500 in_out_cubic;
+    tokens = tokenize(css, __name__)
+    pprint.pp(list(tokens))
diff --git a/tests/test_css_parse.py b/tests/test_css_parse.py
new file mode 100644
index 0000000000..f17a0c3e94
--- /dev/null
+++ b/tests/test_css_parse.py
@@ -0,0 +1,129 @@
+import pytest
+from rich.color import Color, ColorType
+
+from textual.css.scalar import Scalar, Unit
+from textual.css.stylesheet import Stylesheet, StylesheetParseError
+from textual.css.transition import Transition
+
+
+class TestParseText:
+    def test_foreground(self):
+        css = """#some-widget {
+            text: green;
+        }
+        """
+        stylesheet = Stylesheet()
+        stylesheet.parse(css)
+
+        styles = stylesheet.rules[0].styles
+        assert styles.text_color == Color.parse("green")
+
+    def test_background(self):
+        css = """#some-widget {
+            text: on red;
+        }
+        """
+        stylesheet = Stylesheet()
+        stylesheet.parse(css)
+
+        styles = stylesheet.rules[0].styles
+        assert styles.text_background == Color("red", type=ColorType.STANDARD, number=1)
+
+
+class TestParseOffset:
+    @pytest.mark.parametrize("offset_x, parsed_x, offset_y, parsed_y", [
+        ["-5.5%", Scalar(-5.5, Unit.PERCENT, Unit.WIDTH), "-30%", Scalar(-30, Unit.PERCENT, Unit.HEIGHT)],
+        ["5%", Scalar(5, Unit.PERCENT, Unit.WIDTH), "40%", Scalar(40, Unit.PERCENT, Unit.HEIGHT)],
+        ["10", Scalar(10, Unit.CELLS, Unit.WIDTH), "40", Scalar(40, Unit.CELLS, Unit.HEIGHT)],
+    ])
+    def test_composite_rule(self, offset_x, parsed_x, offset_y, parsed_y):
+        css = f"""#some-widget {{
+            offset: {offset_x} {offset_y};
+        }}
+        """
+        stylesheet = Stylesheet()
+        stylesheet.parse(css)
+
+        styles = stylesheet.rules[0].styles
+
+        assert len(stylesheet.rules) == 1
+        assert stylesheet.rules[0].errors == []
+        assert styles.offset.x == parsed_x
+        assert styles.offset.y == parsed_y
+
+    @pytest.mark.parametrize("offset_x, parsed_x, offset_y, parsed_y", [
+        ["-5.5%", Scalar(-5.5, Unit.PERCENT, Unit.WIDTH), "-30%", Scalar(-30, Unit.PERCENT, Unit.HEIGHT)],
+        ["5%", Scalar(5, Unit.PERCENT, Unit.WIDTH), "40%", Scalar(40, Unit.PERCENT, Unit.HEIGHT)],
+        ["-10", Scalar(-10, Unit.CELLS, Unit.WIDTH), "40", Scalar(40, Unit.CELLS, Unit.HEIGHT)],
+    ])
+    def test_separate_rules(self, offset_x, parsed_x, offset_y, parsed_y):
+        css = f"""#some-widget {{
+            offset-x: {offset_x};
+            offset-y: {offset_y};
+        }}
+        """
+        stylesheet = Stylesheet()
+        stylesheet.parse(css)
+
+        styles = stylesheet.rules[0].styles
+
+        assert len(stylesheet.rules) == 1
+        assert stylesheet.rules[0].errors == []
+        assert styles.offset.x == parsed_x
+        assert styles.offset.y == parsed_y
+
+
+class TestParseTransition:
+    @pytest.mark.parametrize(
+        "duration, parsed_duration", [
+            ["5.57s", 5.57],
+            ["0.5s", 0.5],
+            ["1200ms", 1.2],
+            ["0.5ms", 0.0005],
+            ["20", 20.],
+            ["0.1", 0.1],
+        ]
+    )
+    def test_various_duration_formats(self, duration, parsed_duration):
+        easing = "in_out_cubic"
+        transition_property = "offset"
+        css = f"""#some-widget {{
+            transition: {transition_property} {duration} {easing} {duration};
+        }}
+        """
+        stylesheet = Stylesheet()
+        stylesheet.parse(css)
+
+        styles = stylesheet.rules[0].styles
+
+        assert len(stylesheet.rules) == 1
+        assert stylesheet.rules[0].errors == []
+        assert styles.transitions == {
+            "offset": Transition(duration=parsed_duration, easing=easing, delay=parsed_duration)
+        }
+
+    def test_no_delay_specified(self):
+        css = f"#some-widget {{ transition: offset-x 1 in_out_cubic; }}"
+        stylesheet = Stylesheet()
+        stylesheet.parse(css)
+
+        styles = stylesheet.rules[0].styles
+
+        assert stylesheet.rules[0].errors == []
+        assert styles.transitions == {
+            "offset-x": Transition(duration=1, easing="in_out_cubic", delay=0)
+        }
+
+    def test_unknown_easing_function(self):
+        invalid_func_name = "invalid_easing_function"
+        css = f"#some-widget {{ transition: offset 1 {invalid_func_name} 1; }}"
+
+        stylesheet = Stylesheet()
+        with pytest.raises(StylesheetParseError) as ex:
+            stylesheet.parse(css)
+
+        stylesheet_errors = stylesheet.rules[0].errors
+
+        assert len(stylesheet_errors) == 1
+        assert stylesheet_errors[0][0].value == invalid_func_name
+        assert ex.value.errors is not None


Code review comments:
diff_hunk: @@ -0,0 +1,43 @@
+import re
+
+_match_duration = re.compile(r"^(-?\d+\.?\d*)(s|ms)$").match
+
+
+class DurationError(Exception):
+    pass
+
+
+class DurationParseError(DurationError):
+    pass
+
+
+def _duration_as_seconds(duration: str) -> float:
+    """
+    Args:
+        duration (str): A string of the form ``"2s"`` or ``"300ms"``, representing 2 seconds and
+            300 milliseconds respectively. If no unit is supplied, e.g. ``"2"``, then the duration is
+            assumed to be in seconds.
+    Raises:
+        DurationParseError: If the argument ``duration`` is not a valid duration string.
+    Returns:
+        float: The duration in seconds.
+
+    """
+    match = _match_duration(duration)
+
+    if match:
+        value, unit_name = match.groups()
+        value = float(value)
+        if unit_name == "ms":
+            duration_secs = value / 1000
+        else:
+            duration_secs = value
+    else:
+        try:
+            duration_secs = float(duration)
+        except ValueError:
+            raise DurationParseError(
+                f"'{duration}' is not a valid duration."
path: src/textual/_duration.py
text: @user1:
Rather than surrounding duration in quotes, could you repr it with !r

Might not be applicable here, but in general it guarantees that it will format nicely.

diff_hunk: @@ -86,18 +86,18 @@ def read(self, filename: str) -> None:
                 css = css_file.read()
             path = os.path.abspath(filename)
         except Exception as error:
-            raise StylesheetError(f"unable to read {filename!r}; {error}") from None
+            raise StylesheetError(f"unable to read {filename!r};") from error
path: src/textual/css/stylesheet.py
text: @user1:
This particular error should never render as a traceback, so I'd prefer to keep it with the semi-colon delimited errors.