-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MRG+1] fixes #15 with negative look-ahead regexp #16
base: master
Are you sure you want to change the base?
Conversation
@@ -269,7 +269,7 @@ def rule_to_regex(cls, rule): | |||
|
|||
# other | symbols should be escaped | |||
# we have "|$" in our regexp - do not touch it | |||
rule = re.sub("(\|)[^$]", r"\|", rule) | |||
rule = re.sub("(\|)(?!\$)", '\|', rule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 I would add the r
prefix to both strings instead of removing it, for future-proofing. But the suggested changes should work.
rule = re.sub("(\|)(?!\$)", '\|', rule) | |
rule = re.sub(r"(\|)(?!\$)", r'\|', rule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's too long ago. I changed projects long ago. Do I have to accept the suggestions or could you do it as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, don’t worry, and it’s not a deal breaker anyway. I’ll wait for a second approval, though.
PS: Thank you very much for your patch, I’m deeply sorry for the delay.
The modification didn't pass the continuous-integration/travis-ci/pr because of failure on pypy, so it isn't available on current github code. Would you consider fix this? Also, the other pull requests modification isn't available as well. |
This is not blocked by the CI failure, it only requires a second maintainer to approve. |
No description provided.