dualfroz
8db541c87e
Inside a character class '|' is a literal, so [+|-] matched '+', '-', or '|'. A leading pipe was captured as a sign and stripped, letting malformed input like '|5m' parse to 300 instead of returning None. Restrict the class to the two intended sign characters.
59 lines
1.5 KiB
Markdown
59 lines
1.5 KiB
Markdown
# Fix sign character class accidentally matching a pipe `|`
|
|
|
|
## Problem
|
|
|
|
`timeparse` accepts a leading pipe `|` as though it were a valid sign and
|
|
silently parses the rest of the expression, instead of rejecting the malformed
|
|
input with `None`:
|
|
|
|
```python
|
|
>>> from pytimeparse import parse
|
|
>>> parse('|5m')
|
|
300 # expected None
|
|
>>> parse('|1:24')
|
|
84 # expected None
|
|
```
|
|
|
|
## Root cause
|
|
|
|
`pytimeparse/timeparse.py`:
|
|
|
|
```python
|
|
SIGN = r'(?P<sign>[+|-])?'
|
|
```
|
|
|
|
Inside a character class, `|` is a literal character, not the alternation
|
|
operator. So `[+|-]` matches `+`, `-`, or `|`. When the input starts with `|`,
|
|
the `sign` group captures it, the pipe is stripped from the string, and the
|
|
remainder is parsed as if no sign were present (`sign` resolves to `1` because
|
|
`'|' != '-'`).
|
|
|
|
## Fix
|
|
|
|
Use a character class containing only the two intended sign characters:
|
|
|
|
```python
|
|
SIGN = r'(?P<sign>[+-])?'
|
|
```
|
|
|
|
Now a leading `|` is not consumed as a sign; the pipe stays in the string, no
|
|
time-format pattern matches it, and `timeparse` returns `None` as it does for
|
|
any other unparseable input. Valid `+`/`-` signs are unaffected.
|
|
|
|
## Test
|
|
|
|
Extended `test_timeparse_signs` in `pytimeparse/tests/testtimeparse.py`:
|
|
|
|
```python
|
|
self.assertIsNone(timeparse.timeparse('|5m'))
|
|
self.assertIsNone(timeparse.timeparse('| 5m'))
|
|
self.assertIsNone(timeparse.timeparse('|1:24'))
|
|
```
|
|
|
|
## Gate
|
|
|
|
- `python -m pytest pytimeparse/tests/testtimeparse.py` -> 49 passed.
|
|
|
|
Counterfactual (fix reverted, test kept): fails with
|
|
`AssertionError: 300 is not None`.
|