feat: Add support for choices in template components.
#282
Conversation
Reviewer's GuideExtend the Sequence diagram for the updated split filter operationsequenceDiagram
participant TE as TemplateEngine
participant S as split_filter
participant TFN as _to_tuple_if_needed_function
TE->>S: invoke split(value, delimiter)
S->>S: value.split(delimiter) results in items_list
loop for each item in items_list
S->>TFN: _to_tuple_if_needed(item)
alt item matches "Verbose label <value>" format
TFN-->>S: return (value_extracted, label_extracted)
else item does not match
TFN-->>S: return item_as_string
end
end
S-->>TE: return list_of_processed_items
Class diagram for changes in cms_component.pyclassDiagram
class `cms_component.py` {
+split(value: str, delimiter: str = "|") : list /* Python type: list[str | tuple[str,str]] */
-_to_tuple_if_needed(value: str) : object /* Python type: str | tuple[str,str] */
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #282 +/- ##
==========================================
+ Coverage 88.66% 88.68% +0.02%
==========================================
Files 124 124
Lines 3378 3386 +8
Branches 288 289 +1
==========================================
+ Hits 2995 3003 +8
Misses 265 265
Partials 118 118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes - here's some feedback:
- The docstring for _to_tuple_if_needed says it returns a single-element tuple when there’s no match, but the code actually returns the original string—please update the docstring to match the behavior.
- You might simplify the return type annotation on split to list[str | tuple[str, str]] rather than a union of two separate list types for clearer typing.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -49,10 +51,29 @@ def field(context: template.Context, field_name: str, field_type: forms.Field, * | |||
| return "" | |||
|
|
|||
|
|
|||
| def _to_tuple_if_needed(value: str) -> str | tuple[str, str]: | |||
There was a problem hiding this comment.
issue: Docstring for _to_tuple_if_needed misrepresents return value
Update the docstring to reflect that non-matches return a plain string, or modify the function to return a tuple in those cases for consistency.
| @register.filter | ||
| def split(value: str, delimiter: str = "|") -> list[str]: | ||
| def split(value: str, delimiter: str = "|") -> list[str] | list[tuple[str, str]]: |
There was a problem hiding this comment.
issue: Return type annotation for split is too narrow
The annotation should be list[str | tuple[str, str]] to accurately reflect that the list may contain both types, not a union of two separate list types.
| @@ -69,7 +69,23 @@ def test_split_template_tag(self): | |||
| self.assertEqual(split("hero.html"), ["hero.html"]) | |||
There was a problem hiding this comment.
nitpick (testing): This test case for split("hero.html") appears to be a duplicate.
This assertion duplicates the one above; consider removing it unless it tests a different scenario.
| .. note:: | ||
|
|
||
| For translators it is important to know, that they **should not translate** the value in angle brackets. | ||
| The German translation of the above example string might be ``Rot <red>|Grün <green>|Standard <blue>``. |
There was a problem hiding this comment.
issue (typo): Typo: extraneous comma after "know".
Remove the comma after "know" for correct grammar: "For translators it is important to know that they should not translate the value in angle brackets."
| .. note:: | |
| For translators it is important to know, that they **should not translate** the value in angle brackets. | |
| The German translation of the above example string might be ``Rot <red>|Grün <green>|Standard <blue>``. | |
| .. note:: | |
| For translators it is important to know that they **should not translate** the value in angle brackets. | |
| The German translation of the above example string might be ``Rot <red>|Grün <green>|Standard <blue>``. |
| match = re.match(r"^(.*?)\s*<([^>]+)>\s?$", value) | ||
| if match: | ||
| return (match.group(2).strip(), match.group(1).strip()) |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace m.group(x) with m[x] for re.Match objects [×2] (
use-getitem-for-re-match-groups)
| match = re.match(r"^(.*?)\s*<([^>]+)>\s?$", value) | |
| if match: | |
| return (match.group(2).strip(), match.group(1).strip()) | |
| if match := re.match(r"^(.*?)\s*<([^>]+)>\s?$", value): | |
| return match[2].strip(), match[1].strip() |
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes - here's some feedback:
- The
_to_tuple_if_neededdocstring says non‐matching strings are returned as a single‐element tuple but the code actually returns the raw string—please update the doc or implementation to match. - The return type on
split(list[str] | list[tuple[str, str]]) doesn’t allow for mixed lists of strings and tuples; consider usinglist[str | tuple[str, str]]to be accurate. - The current regex will misinterpret strings with extra
<or>characters—either refine the pattern for edge cases or document its limitations explicitly.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| match = re.match(r"^(.*?)\s*<([^>]+)>\s?$", value) | ||
| if match: | ||
| return (match.group(2).strip(), match.group(1).strip()) |
There was a problem hiding this comment.
suggestion (performance): Consider precompiling the regex pattern or using re.fullmatch
Compiling the regex once at module scope improves efficiency. Also, re.fullmatch can simplify the pattern by removing the need for explicit anchors.
| match = re.match(r"^(.*?)\s*<([^>]+)>\s?$", value) | |
| if match: | |
| return (match.group(2).strip(), match.group(1).strip()) | |
| import re | |
| _EMAIL_CHOICE_PATTERN = re.compile(r"(.*?)\s*<([^>]+)>\s?") | |
| match = _EMAIL_CHOICE_PATTERN.fullmatch(value) | |
| if match: | |
| return (match.group(2).strip(), match.group(1).strip()) |
|
|
||
| Returns: | ||
| tuple[str, str]: A tuple containing the two parts of the string if it contains a delimiter, | ||
| otherwise returns the original string as a single-element tuple. |
There was a problem hiding this comment.
issue: Docstring return description is inaccurate
Update the docstring to reflect that non-matching inputs return the raw string, or modify the code to return a single-element tuple as described.
| @register.filter | ||
| def split(value: str, delimiter: str = "|") -> list[str]: | ||
| def split(value: str, delimiter: str = "|") -> list[str] | list[tuple[str, str]]: |
There was a problem hiding this comment.
suggestion: Type annotation does not reflect mixed element types
Annotate the return type as list[str | tuple[str, str]] to accurately represent mixed element types.
| @register.filter | |
| def split(value: str, delimiter: str = "|") -> list[str]: | |
| def split(value: str, delimiter: str = "|") -> list[str] | list[tuple[str, str]]: | |
| @register.filter | |
| def split(value: str, delimiter: str = "|") -> list[str | tuple[str, str]]: |
| @@ -61,4 +82,5 @@ def split(value: str, delimiter: str = "|") -> list[str]: | |||
| Returns: | |||
| list[str]: A list of substrings obtained by splitting the input string using the delimiter. | |||
There was a problem hiding this comment.
issue: Update Returns doc to include tuple cases
Please revise the Returns section to document that the function may also return tuples, not just a list of strings.
| @@ -49,10 +51,29 @@ def field(context: template.Context, field_name: str, field_type: forms.Field, * | |||
| return "" | |||
|
|
|||
|
|
|||
| def _to_tuple_if_needed(value: str) -> str | tuple[str, str]: | |||
There was a problem hiding this comment.
issue (complexity): Consider replacing the regex-based parsing in _to_tuple_if_needed with simple string methods to reduce complexity and remove the need for the re import.
| def _to_tuple_if_needed(value: str) -> str | tuple[str, str]: | |
| You can remove the regex import and simplify `_to_tuple_if_needed` using built-in string methods. For example: | |
| ```python | |
| def _to_tuple_if_needed(item: str) -> str | tuple[str, str]: | |
| # Split on the last '<'; if not found or no '>' after it, just return the original | |
| name, sep, rest = item.rpartition('<') | |
| if not sep or '>' not in rest: | |
| return item | |
| # Extract the value up to the first '>' and strip whitespace | |
| val, _sep2, _ = rest.partition('>') | |
| return (val.strip(), name.strip()) |
This removes import re and avoids the regex overhead.
Optionally, you can inline the same logic right in the filter to keep everything in one place:
@register.filter
def split(value: str, delimiter: str = "|") -> list[str] | list[tuple[str, str]]:
parts = value.split(delimiter)
out = []
for item in parts:
name, sep, rest = item.rpartition('<')
if sep and '>' in rest:
val, _sep2, _ = rest.partition('>')
out.append((val.strip(), name.strip()))
else:
out.append(item.strip())
return outBoth approaches preserve your functionality but reduce complexity by using simple string operations.
| match = re.match(r"^(.*?)\s*<([^>]+)>\s?$", value) | ||
| if match: | ||
| return (match.group(2).strip(), match.group(1).strip()) |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace m.group(x) with m[x] for re.Match objects [×2] (
use-getitem-for-re-match-groups)
| match = re.match(r"^(.*?)\s*<([^>]+)>\s?$", value) | |
| if match: | |
| return (match.group(2).strip(), match.group(1).strip()) | |
| if match := re.match(r"^(.*?)\s*<([^>]+)>\s?$", value): | |
| return match[2].strip(), match[1].strip() |
The
choicesparameter of aChoiceFieldrequires a list of 2-tuples(value, verbose_string). This PR extends thesplittemplate tag to turn substring of the form"Verbose label <value>"into 2-tuples:lets
choicesbecomeSummary by Sourcery
Enable parsing of choice definitions in the split template filter to support Django ChoiceField syntax.
New Features:
Enhancements:
_to_tuple_if_neededwith regex matching to extract verbose labels and values.__future__.annotationsimport and update type hints on the split filter.Tests: