8000 Add escape filter by morenol · Pull Request #144 · jinja2cpp/Jinja2Cpp · GitHub
[go: up one dir, main page]
Skip to content

Add escape filter #144

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 27, 2019
Merged

Add escape filter #144

merged 1 commit into from
Sep 27, 2019

Conversation

morenol
Copy link
Contributor
@morenol morenol commented Sep 26, 2019 •
edited
Loading

Solves #140

Still work in progress, not sure how to test the quotes (‘, ”).

@codecov
Copy link
codecov bot commented Sep 26, 2019 •
edited
Loading

Codecov Report

Merging #144 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   87.26%   87.29%   +0.02%     
==========================================
  Files          64       64              
  Lines        7219     7226       +7     
==========================================
+ Hits         6300     6308       +8     
+ Misses        919      918       -1
Impacted Files Coverage Δ
src/string_converter_filter.cpp 93.16% <100%> (+0.31%) ⬆️
test/filters_test.cpp 100% <100%> (ø) ⬆️
src/helpers.h 96.77% <0%> (+3.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3dc7857...ec1c9de. Read the comment docs.

@flexferrum
Copy link
Collaborator

Still work in progress, not sure how to test the quotes (‘, ”).

Thanks for contributing! What the exact problem with test for quotes?

@morenol
Copy link
Contributor Author
morenol commented Sep 26, 2019

Still work in progress, not sure how to test the quotes (‘, ”).

Thanks for contributing! What the exact problem with test for quotes?

I think that I solved that, I had to escape both the quotes and the backslash
InputOutputPair{"'\\\"\\'' | escape | pprint", "'&#34;&#39;'"}

@morenol morenol changed the title [WIP] Add escape filter Add escape filter Sep 26, 2019
@flexferrum
Copy link
Collaborator

I think that I solved that, I had to escape both the quotes and the backslash
InputOutputPair{"'\\\"\\'' | escape | pprint", "'&#34;&#39;'"}

Another approach is to use raw-string literals: InputOutputPair{R"('\"\'' | escape | pprint)", "'&#34;&#39;'"}

But your current solution is ok as well.

@flexferrum
Copy link
Collaborator

@morenol , sorry, I made a mistake when was resolving conflicts of your PR and new master. I missed the break statement. Could you provide a fix?

@morenol
Copy link
Contributor Author
morenol commented Sep 26, 2019

Done!

@flexferrum
Copy link
Collaborator

Thanks!

flexferrum
flexferrum approved these changes Sep 26, 2019
View reviewed changes
Copy link
Collaborator
@flexferrum flexferrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flexferrum
Copy link
Collaborator

@morenol I can merge your PR now or wait till the October, 1st. What would your prefer?

@morenol
Copy link
Contributor Author
morenol commented Sep 26, 2019

Merge, I hope to do more of them in October!

@morenol
Copy link
Contributor Author
morenol commented Sep 26, 2019

I noticed that I haven't edited the readme, I'm going to do that, Is it ok?

@flexferrum
Copy link
Collaborator
flexferrum commented Sep 26, 2019 •
edited
Loading

I noticed that I haven't edited the readme, I'm going to do that, Is it ok?

It's ok. I'm preparing the big PR with preparation to the version 1.0.0 release, so I'll edit the readme in any case.

@flexferrum flexferrum merged commit ac607d2 into jinja2cpp:master Sep 27, 2019
@morenol morenol deleted the lmm/escape-filter branch September 27, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flexferrum flexferrum flexferrum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@morenol @flexferrum
0