diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..707c67c --- /dev/null +++ b/PR.md @@ -0,0 +1,58 @@ +# 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[+|-])?' +``` + +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[+-])?' +``` + +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`. diff --git a/pytimeparse/tests/testtimeparse.py b/pytimeparse/tests/testtimeparse.py index c305bb5..29f4cef 100644 --- a/pytimeparse/tests/testtimeparse.py +++ b/pytimeparse/tests/testtimeparse.py @@ -83,6 +83,10 @@ class TestTimeparse(unittest.TestCase): self.assertEqual(timeparse.timeparse('- 32 m 1 s'), -1921) self.assertIsNone(timeparse.timeparse('32 m - 1 s')) self.assertIsNone(timeparse.timeparse('32 m + 1 s')) + # A stray pipe is not a valid sign and must not be silently accepted. + self.assertIsNone(timeparse.timeparse('|5m')) + self.assertIsNone(timeparse.timeparse('| 5m')) + self.assertIsNone(timeparse.timeparse('|1:24')) def test_timeparse_1(self): '''timeparse test case 1.''' diff --git a/pytimeparse/timeparse.py b/pytimeparse/timeparse.py index ace5cc3..346596f 100644 --- a/pytimeparse/timeparse.py +++ b/pytimeparse/timeparse.py @@ -33,7 +33,7 @@ kinds of time expressions. import re -SIGN = r'(?P[+|-])?' +SIGN = r'(?P[+-])?' #YEARS = r'(?P\d+)\s*(?:ys?|yrs?.?|years?)' #MONTHS = r'(?P\d+)\s*(?:mos?.?|mths?.?|months?)' WEEKS = r'(?P[\d.]+)\s*(?:w|wks?|weeks?)'