Issue #87 Make sanitization happen in Post.htmlize() #88
Conversation
janetriley
commented
Jun 4, 2017
janetriley
commented
- Made utils/text. Moved sanitize functions, constants, and tests there.
- Post.htmlize() accepts and applies a sanitization param
- Export passes along the level to to Post.
* Made utils/text. Moved sanitize functions, constants, and tests there. * Post.htmlize() accepts and applies a sanitization param * Export passes along the level to to Post.
There was a problem hiding this comment.
@janetriley thank you so much, you're absolutely right, this was a good thing to do and I'm glad to have this functionality. I had some questions and comments below, mind taking a gander at them and letting me know your thoughts? Then I can approve/merge.
baleen/models.py
Outdated
| """ | ||
| return self.content | ||
|
|
There was a problem hiding this comment.
I really like what you've done here, it is extremely sensible. However, I'd ask for one minor change so that this is not a complete passthrough to the sanitize function. What would you think about changing the signature to accept None as the argument to level and changing the level argument to sanitize, e.g.:
def htmlize(self, sanitize=None):
"""
...
:param sanitize: str or None, default None
Specify a level to sanitize the HTML. If ``None``, then no HTML sanitization will
be conducted. Otherwise, specify ``'raw'``, ``'safe'``, or ``'text'`` to sanitize
the HTML at different levels. See ref::``sanitize_html`` for more.
"""
if sanitize:
return sanitize_html(html=self.content, level=sanitize)
return self.content I think this makes it a bit more understandable to users, what do you think?
baleen/utils/text.py
Outdated
| ) | ||
|
|
||
|
|
||
| RAW = 'raw' |
There was a problem hiding this comment.
These module constants should be hoisted to the top of the file.
tests/utils_tests/test_text.py
Outdated
|
|
||
| import unittest | ||
|
|
||
| from mongomock import MongoClient as MockMongoClient |
There was a problem hiding this comment.
do you need MockMongoClient here?
tests/utils_tests/test_text.py
Outdated
|
|
||
| from baleen.exceptions import ExportError | ||
| from baleen.export import MongoExporter | ||
| from baleen.models import connect |
tests/utils_tests/test_text.py
Outdated
| from baleen.export import MongoExporter | ||
| from baleen.models import connect | ||
| from baleen.utils.text import sanitize_html, RAW, SAFE, TEXT | ||
| from tests.test_export import CATEGORIES_IN_DB |
| @@ -254,57 +247,3 @@ def test_export_with_category_path_failure(self): | |||
| exporter.export() | |||
|
|
|||
|
|
|||
| class SanitizeHtmlTests(unittest.TestCase): | |||
There was a problem hiding this comment.
It absolutely makes sense to remove these tests, should we have a test for Post.htmlize()? It will just be mocking sanitize_html and making sure it is called with the right arguments. If you think that's overkill though I understand.
There was a problem hiding this comment.
It's verging on overkill but I like making percentages go up. Let me look at it this weekend.
There was a problem hiding this comment.
Hehe, ok -- well do you want me to merge this PR and make a new one, or would you rather keep this PR open until you write the tests?
BTW - do you see a button at the bottom entitled "squash and merge", once everything is reviewed and CI approved, please feel free to merge yourself if you do see that button.
There was a problem hiding this comment.
I'll merge this and take up the test in another feature, possibly for #89 .
Clean up imports and comments in tests.
* Add docstrings to the top * move constants to top * rename _get_ methods to get_ - they're not hidden inside a class, may as well make them available directly to those that want it
|
@bbengfort, a couple questions: How do you generate the ID numbers at the opening comments, like line 10 in utils/timez.py? |
