-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
replacer: Fix escaped closing braces #5995
replacer: Fix escaped closing braces #5995
Conversation
Thank you for looking into the fix! Would you mind adding test cases? You can consider the reported config/behavior as one. If you can come up with more scenarios, that'd be great. |
6acb8fc
to
2e5e63a
Compare
@mohammed90 I have a question. One of the tests that are currently present (And which I had to change to pass the checks) states that I am asking because I may be missing an intended behavior here. Thank you. |
Checking git blame on that line shows it was added as part of PR #3121 to resolve #3116. I believe it's intended, but I'm not firm on this. I need to think about it, especially because that area of the code is not very familiar to me. Perhaps Francis or Matt can chime in. |
Yeah looks weird to me, but also I don't think escaping was necessary at all in the reported issue. I don't have any objections to this, collapsing escapes seems fine to me here (as long as But yeah we should have some more test cases to make sure this change is bulletproof. |
Thank you guys for the prompt responses. I will work on adding more test cases. |
I just added a couple tests. Please let me know if you want to see more cases covered. Thank you! |
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.
Looking good, just a few minor nitpicks from my side.
13cd757
to
8a154ba
Compare
@mohammed90 Thank you for taking the time to review! All your feedback points should now be addressed. |
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.
LGTM, thanks!
8a154ba
to
34b980a
Compare
@armadi1809 Thank you for your contribution! Please feel welcome to contribute again :) |
Resolves #5993. Inside
func (r *Replacer) replace
we were returning the same input if it does not contain the opening brace but which meant that we were not treating the closing brace as a character that should be escaped.