refactor(utils): extract shared truncate() helper, replace six inline _truncate() implementations#1080
Conversation
…ate() implementations
Greptile SummaryThis PR extracts six identical private Confidence Score: 5/5Safe to merge; all six migrations are behaviourally equivalent and the only findings are P2 style/edge-case issues. All remaining findings are P2: an unguarded negative-slice edge case that no current caller can trigger, and a parameter name that shadows a built-in. No correctness regressions were introduced. app/utils/truncation.py — minor edge-case guard and parameter naming. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph callers["Call sites"]
F["discord_delivery (ellipsis=…)"]
G["telegram_delivery (ellipsis=…)"]
H["gitlab_writeback (ellipsis=...)"]
I["azure_sql (ellipsis=...)"]
J["mysql (ellipsis=...)"]
K["mariadb (ellipsis=...)"]
end
callers --> A["truncate(text, limit, ellipsis)"]
A --> B{len text <= limit}
B -- yes --> C[return text unchanged]
B -- no --> D["text[ : limit - len(ellipsis)] + ellipsis"]
D --> E[return truncated string]
Reviews (1): Last reviewed commit: "refactor: extract shared truncate() help..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def truncate(text: str, limit: int, ellipsis: str = "...") -> str: | ||
| if len(text) <= limit: | ||
| return text | ||
| return text[: limit - len(ellipsis)] + ellipsis |
There was a problem hiding this comment.
limit < len(ellipsis)
When limit is smaller than the length of the ellipsis string, text[:limit - len(ellipsis)] evaluates to a negative index (text[:-N]), stripping characters from the end of the original text and then appending the full ellipsis — producing a result longer than limit. For example, truncate("hello", 2, "...") returns "hel..." (6 chars) instead of the expected 2-char result. None of the current callers hit this case, but the helper has no guard and the test suite doesn't cover it.
| return text[: limit - len(ellipsis)] + ellipsis | |
| return text[: max(0, limit - len(ellipsis))] + ellipsis |
| """Shared text truncation utility.""" | ||
|
|
||
|
|
||
| def truncate(text: str, limit: int, ellipsis: str = "...") -> str: |
There was a problem hiding this comment.
ellipsis shadows the Python built-in
The parameter name ellipsis shadows the Python built-in singleton of the same name. While it doesn't cause a bug here, it's a lint target under ruff's A002 (argument-shadows-builtin) rule. A name like suffix or trail avoids the shadow without changing the public API concept.
|
@muddlebee all six inline _truncate() implementations replaced with the shared helper in app/utils/truncation.py. Renamed ellipsis param to suffix to avoid the builtin shadow, added max(0, ...) guard when limit is smaller than the suffix. Ready for review |
| def truncate(text: str, limit: int, suffix: str = "...") -> str: | ||
| if len(text) <= limit: | ||
| return text | ||
| cut = max(0, limit - len(suffix)) |
There was a problem hiding this comment.
When limit < len(suffix) (e.g. truncate("abc", 2, "...")), cut=0 and the return value is just suffix (length 3), which silently exceeds limit.
Consider all the edge cases here.. and add proper unit tests for all edge cases.
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "text,limit,ellipsis,expected", |
There was a problem hiding this comment.
The parametrize header uses ellipsis as a field name — it shadows the Python built-in. Rename to suffix to match the actual parameter name and avoid any future confusion.
…tlab body capped at 4000
|
@muddlebee addressed both comments — renamed ellipsis to suffix in the parametrize header, added limit < len(suffix) edge case handling (return suffix[:limit]), and added full edge case test coverage including limit=0, empty |
|
@udit-rawat nice work. 🚀 |
|
🎊 Achievement unlocked: PR Merged. @udit-rawat passed code review, survived CI, and shipped. Respect. 🤝
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
2 participants
Problem
Six modules independently defined a private
_truncate()function with inconsistent implementations — different ellipsis characters (...vs…), different limits (200, 500, 4000, 4096), and no shared source of truth. Every new delivery channel or integration had to reinvent the same logic.Affected files:
discord_delivery.py,telegram_delivery.py,azure_sql.py,mysql.py,mariadb.py, andgitlab_writeback.py.Solution
Added
app/utils/truncation.pywith a singletruncate(text, limit, ellipsis="...")helper. Migrated all six call sites to use it. Each module's limit constant stays where it is — only the function body moves.Tests
Added
tests/utils/test_truncation.pycovering:…)Closes #1056