allowing Matches to accept re.compile
#135
Conversation
bandophahita
changed the title
re.compileMatches to accept re.compile
| if isinstance(self.pattern, Pattern): | ||
| return f"r'{self.pattern.pattern}'" | ||
| return f"r'{self.pattern}'" |
There was a problem hiding this comment.
Do we want to move this logic to represent_prop?
There was a problem hiding this comment.
There is a slight issue with doing that. I thought it best to show the pattern in the log as r'pattern', which includes the r before the string quotes.
... hoping it's text matching the pattern r'\tpattern'. -- GOOD
... hoping it's text matching the pattern '\\tpattern'. -- EW!
... hoping it's text matching the pattern ' pattern'. -- BAD!
This is doable with Pattern objects but when the pattern is a string, there is no way for represent_prop to differentiate r-strings from regular strings.
@property
def pattern_to_log(self) -> str:
"""Represent the item in a log-friendly way."""
return represent_prop(self.pattern)The above will fail the logging tests:
Expected :"Text matching the pattern r'(spam)+'."
Actual :"Text matching the pattern '(spam)+'."
To counter this, I considered adding a switch argument to represent_prop which would gives the caller the option to turn on/off the r in front of strings but I felt like this was an over complicated solution for a single prop.
There was a problem hiding this comment.
I still think we should move this logic to represent_prop, we have other Pattern-accepting -ables.
Maybe we can do something like this, here:
def pattern_to_log(self):
"""Represent the item in a log-friendly way."""
loggable_pattern = represent_prop(self.pattern)
if not loggable_pattern.startswith("r"):
loggable_pattern = f"r'{loggable_pattern}'"
return loggable_patternHow does that sit with you?
There was a problem hiding this comment.
When self.pattern is a string, represent_prop will call repr
if isinstance(item, str):
return repr(item)Which is unfortunately not how we want the pattern to appear in the log.
represent_prop(r"\tpattern") would return "\\tpattern". Putting an r in front of the string wouldn't fix that.
|
@perrygoy back to you. |
bandophahita
force-pushed
the
regex_resolution_update
branch
from
f1925f4 to
5dd6f5e
Compare
| def test_the_test_with_compile(self) -> None: | ||
| m = Matches(re.compile(r"([Ss]pam ?)+")).resolve() | ||
|
|
||
| assert m.matches("Spam spam spam spam baked beans and spam") | ||
| assert not m.matches("What do you mean Eugh?!") |
There was a problem hiding this comment.
I personally wouldn't mind having this be part of the "test the test" test above, that's the pattern we've followed in all the other tests. But i don't mind it being a separate test.
| def test_description_with_compile(self) -> None: | ||
| test_match = re.compile(r"(spam)+") | ||
|
|
||
| m = Matches(test_match) | ||
|
|
||
| expected_description = "Text matching the pattern r'(spam)+'." |
There was a problem hiding this comment.
Same goes for this test; we've just grouped all the "describe correctly" tests in one in other ones (though currently the only examples i can think of are for testing SeeAllOf and SeeAnyOf).
The resolving function
matches_regexpofMatchescan accept eitherstrorPattern. This update makes it possible to pass the pattern directly intoMatches.