Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't expand unused vars #79

Merged
merged 2 commits into from
Jun 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 79 additions & 7 deletions src/tinuous/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
import os
from pathlib import Path
import re
from string import Formatter
import subprocess
from typing import Dict, Iterator, cast
from typing import Any, Dict, Iterator, Mapping, Optional, Sequence, Union

import requests

Expand Down Expand Up @@ -46,17 +47,88 @@ def iterfiles(dirpath: Path) -> Iterator[Path]:
yield p


class LazySlicingFormatter(Formatter):
jwodder marked this conversation as resolved.
Show resolved Hide resolved
"""
A `string.Formatter` subclass that:

- accepts a second set of format kwargs that can refer to the main kwargs
or each other and are only templated as needed
- supports indexing strings & other sequences with slices
"""

def __init__(self, var_defs: Dict[str, str]):
self.var_defs: Dict[str, str] = var_defs
self.expanded_vars: Dict[str, str] = {}
super().__init__()

def get_value(
self, key: Union[int, str], args: Sequence[Any], kwargs: Mapping[str, Any]
) -> Any:
if isinstance(key, int):
return args[key]
elif key in kwargs:
return kwargs[key]
elif key in self.expanded_vars:
return self.expanded_vars[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh -- that seems like quite an elaborate (smells like a more proper of cause) solution! ;) Since would be in the "core" of things, could you add unittests for not yet covered cases in this function?

elif key in self.var_defs:
self.expanded_vars[key] = self.format(self.var_defs[key], **kwargs)
return self.expanded_vars[key]
else:
raise KeyError(key)

def get_field(
self, field_name: str, args: Sequence[Any], kwargs: Mapping[str, Any]
) -> Any:
m = re.match(r"\w+", field_name)
assert m, f"format field name {field_name!r} does not start with arg_name"
key: Union[int, str] = m.group()
if key.isdigit():
key = int(key)
obj = self.get_value(key, args, kwargs)
s = field_name[m.end() :]
while s:
m = re.match(r"\.(?P<attr>\w+)|\[(?P<index>[^]]+)\]", s)
assert m, f"format field name {field_name!r} has invalid attr/index"
s = s[m.end() :]
attr, index = m.group("attr", "index")
if attr is not None:
obj = getattr(obj, attr)
else:
assert index is not None # type: ignore[unreachable]
try:
sl = parse_slice(index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to push on subclassing string.Formatter whenever we were talking about enhancing the extra formatters (like for shortened commit) since iirc it was lacking slicing, so here you are (re)implementing it?
Please add tests to cover except clause here and common cases like [-3:] and [start:end].

On a positive side -- this would open the possibility for custom formatter directives (in addition to r) ;-) ! (I am still wondering about git describe for commits ;))

except ValueError:
if index.isdigit():
obj = obj[int(index)]
else:
obj = obj[index]
else:
obj = obj[sl]
return obj, key


def expand_template(
template_str: str, fields: Dict[str, str], vars: Dict[str, str]
) -> str:
expanded_vars: Dict[str, str] = {}
for name, tmplt in vars.items():
expanded_vars[name] = fstring(tmplt, **fields, **expanded_vars)
return fstring(template_str, **fields, **expanded_vars)
return LazySlicingFormatter(vars).format(template_str, **fields)


SLICE_RGX = re.compile(r"(?P<start>-?\d+)?:(?P<stop>-?\d+)?(?::(?P<step>-?\d+)?)?")


def fstring(s: str, **kwargs: str) -> str:
return cast(str, eval(f"f{s!r}", {}, kwargs))
def parse_slice(s: str) -> slice:
if m := SLICE_RGX.fullmatch(s):
s_start, s_stop, s_step = m.group("start", "stop", "step")
start: Optional[int] = None if s_start is None else int(s_start)
stop: Optional[int] = None if s_stop is None else int(s_stop)
step: Optional[int]
if s_step is None or s_step == "":
step = None
else:
step = int(s_step)
return slice(start, stop, step)
else:
raise ValueError(s)


def get_github_token() -> str:
Expand Down
71 changes: 70 additions & 1 deletion test/test_util.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
from tinuous.util import expand_template
from types import SimpleNamespace
from typing import Any, Dict
import pytest
from pytest_mock import MockerFixture
from tinuous.util import LazySlicingFormatter, expand_template, parse_slice


def test_expand_template() -> None:
Expand All @@ -24,3 +28,68 @@ def test_expand_template_sliced() -> None:
)
== "1234567/A test"
)


def test_expand_template_unused_bad() -> None:
assert (
expand_template(
"{cleesh}",
{"description": "A test commit"},
{"bad": "{undefined}", "cleesh": "{description[:6]}"},
)
== "A test"
)


@pytest.mark.parametrize(
"s,sl",
[
(":", slice(None)),
("::", slice(None)),
("23:", slice(23, None)),
("23::", slice(23, None)),
(":42", slice(42)),
(":42:", slice(42)),
("23:42", slice(23, 42)),
("23:42:", slice(23, 42)),
("::5", slice(None, None, 5)),
("23::5", slice(23, None, 5)),
(":42:5", slice(None, 42, 5)),
("23:42:5", slice(23, 42, 5)),
("-23:-42:-5", slice(-23, -42, -5)),
],
)
def test_parse_slice(s: str, sl: slice) -> None:
assert parse_slice(s) == sl


@pytest.mark.parametrize(
"fmt,args,kwargs,result",
[
("{0}", ["foo"], {}, "foo"),
(
"{foo.bar.baz}",
[],
{"foo": SimpleNamespace(bar=SimpleNamespace(baz="quux"))},
"quux",
),
("{foo[1][2]}", [], {"foo": ["abc", "def", "ghi"]}, "f"),
("{foo[bar][baz]}", [], {"foo": {"bar": {"baz": "quux"}}}, "quux"),
],
)
def test_lazy_slicing_formatter_basics(
fmt: str, args: list, kwargs: Dict[str, Any], result: str
) -> None:
assert LazySlicingFormatter({}).format(fmt, *args, **kwargs) == result


def test_lazy_slicing_formatter_undef_key() -> None:
with pytest.raises(KeyError):
LazySlicingFormatter({}).format("{foo}", bar=42)


def test_lazy_slicing_formatter_var_reuse(mocker: MockerFixture) -> None:
fmter = LazySlicingFormatter({"foo": "bar"})
spy = mocker.spy(fmter, "format")
assert fmter.format("-{foo}-{foo}-") == "-bar-bar-"
assert spy.call_args_list == [mocker.call("-{foo}-{foo}-"), mocker.call("bar")]
1 change: 1 addition & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ minversion = 3.3.0
deps =
pytest~=6.0
pytest-cov~=2.0
pytest-mock~=3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to myself: apparently we do not have tests extra_requires and all test requirements are set in tox making it the only kosher way to run tests.

commands =
# Basic smoketest:
tinuous --help
Expand Down