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

Use a custom escaping function instead of lodash for unescaping iomd #2526

Merged
merged 3 commits into from
Dec 16, 2019

Conversation

wlach
Copy link
Contributor

@wlach wlach commented Dec 6, 2019

This fixes one edge case which should be very rare (where an
unescaped sequence becomes an escape sequence after the lodash
transformation)

This fixes one edge case which should be very rare (where an
unescaped sequence becomes an escape sequence after the lodash
transformation)
@wlach wlach self-assigned this Dec 6, 2019
@wlach wlach requested a review from mdboom December 6, 2019 22:49
@wlach
Copy link
Contributor Author

wlach commented Dec 6, 2019

I realized later today that my earlier solution could fail in an obscure edge case. No rush on looking at / merging this one, since I think that should be exceedingly rare.

@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #2526 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2526     +/-   ##
=========================================
+ Coverage   70.99%   71.09%   +0.1%     
=========================================
  Files         239      238      -1     
  Lines        5874     5843     -31     
  Branches      938      935      -3     
=========================================
- Hits         4170     4154     -16     
+ Misses       1685     1670     -15     
  Partials       19       19
Impacted Files Coverage Δ
src/editor/initialization/handle-initial-iomd.js 100% <100%> (ø) ⬆️
server/tests/test_notebook_view.py 98.7% <100%> (ø) ⬆️
src/editor/state-schemas/state-schema.js 87.5% <0%> (ø) ⬆️
src/editor/state-schemas/language-definitions.js 83.33% <0%> (ø) ⬆️
src/eval-frame/code-completion-request-response.js

This would let us catch issues like iodide-project#2524 due to Django upgrades in CI
@wlach wlach merged commit dfa8082 into iodide-project:master Dec 16, 2019
@wlach wlach deleted the refined-escape-logic branch December 16, 2019 16:36
@wlach
Copy link
Contributor Author

wlach commented Dec 16, 2019

As a last-minute addition, I changed the server-side unit test to expect a particular type of escaping, rather than just trusting the output of Django's escape function would be ok. This would have caught the original problem before we merged

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

Successfully merging this pull request may close these issues.

2 participants