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

feat: add an option to preserve whitespace to FullSanitizer #157

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

Conversation

Earlopain
Copy link

Closes #154

There are still a few things missing. @flavorjones perhaps you want to give some feedback in advance?

How thorough should I be for these tests? Ideally Loofah already has good coverage, I don't want to duplicate that unnecessarily. I do however assert that escaping is identical for both methods which I thought important. I added a test helper for that and tweaked a small amount of tests so that there are no output differences because of whitespace.

Docs are missing, will get to those.

@flavorjones
Copy link
Member

Hey, @Earlopain, sorry for my slow reply. Give me a day to come up for air and I'll be able to reply thoughtfully.

@Earlopain Earlopain force-pushed the full-sanitizer-preserve-whitespace branch from c8afae5 to 48d2142 Compare May 16, 2023 06:41
@Earlopain
Copy link
Author

It's all good, no worries. I admit I am a bit lost with the recent changes to main, I'm not sure how I should integrate the changes now.

@Earlopain Earlopain force-pushed the full-sanitizer-preserve-whitespace branch from 48d2142 to 82fea0e Compare May 16, 2023 08:03
@Earlopain Earlopain force-pushed the full-sanitizer-preserve-whitespace branch from 82fea0e to 9a336ad Compare May 16, 2023 08:04
@Earlopain Earlopain marked this pull request as ready for review May 16, 2023 08:10
@Earlopain
Copy link
Author

Earlopain commented May 16, 2023

After some deliberation I have something that at least works. Sorry about the notifications for force-pushing a bunch. It should now be in about the same state it was before, just pointing at current main.

I'm not a big fan how I did it with the module but that was the first and only thing that came to my mind. Feels like something I would monkeypatch when I don't control the code myself.

I do like how the tests are structured now, nice job on that.

I removed the html4 example from the readme, since just one section above it mentions that the examples work both with html4 and html5. That way I only need to add the new argument to one example. There also appears to be stray HTML5 version: below which I removed.

@flavorjones
Copy link
Member

Note to casual readers: a conversation is ongoing about this topic at the original issue #154

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.

Add an option to use to_text instead of to_html to FullSanitizer
2 participants