search

rails / rails-html-sanitizer

MIT License
302 stars 80 forks source link

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

Open Earlopain opened 1 year ago

Earlopain commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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