Skip to content
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

Added xml escaping as a function with replace function #504

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Joostluijben
Copy link

We found out the dotliquid does not support the xml escaping yet like in ruby . Therefore I implemented the function with only replace functions like shown here. I used the replace function because of the discussion found on this stackoverflow post

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Base: 88.26% // Head: 88.06% // Decreases project coverage by -0.19% ⚠️

Coverage data is based on head (3023703) compared to base (0d510d8).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head 3023703 differs from pull request most recent head 61b6c40. Consider uploading reports for the commit 61b6c40 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
- Coverage   88.26%   88.06%   -0.20%     
==========================================
  Files          69       70       +1     
  Lines        2675     2681       +6     
  Branches      620      621       +1     
==========================================
  Hits         2361     2361              
- Misses        210      216       +6     
  Partials      104      104              
Impacted Files Coverage Δ
src/DotLiquid/RubyFilters.cs 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@microalps
Copy link
Contributor

  1. Please include tests to validate this new functionality. If this is in Ruby Liquid, needs to have a c# of all tests included from Ruby.
  2. Makes sense to place in ExtendedFilters or ShopifyFilters and not in it's own RubyFilters

@microalps microalps self-requested a review December 7, 2022 15:24
@microalps
Copy link
Contributor

@Joostluijben We appreciate contributions, but please see my comments above. Also, codecov indicates that testing decreased, so please add whatever tests are needed to validate the functionality works as expected.

Copy link
Contributor

@microalps microalps left a comment

Choose a reason for hiding this comment

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

Kindly see above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants