parse charset and decode text #11
Conversation
|
|
||
| @CachedProperty | ||
| def text(self): | ||
| if not self.media_type.startswith('text/'): |
There was a problem hiding this comment.
intuitively i'd say utf-8 is a saner and more useful default. or does the spec explicitly forbid that?
There was a problem hiding this comment.
Yes, it is:
If one is not specified, the media type of the data URI is assumed to be text/plain;charset=US-ASCIIQ
| @@ -6,3 +6,6 @@ deps=-rrequirements-test.txt | |||
| commands= | |||
| pytest --cov {envsitepackagesdir}/datauri {posargs} tests/ | |||
| flake8 datauri/ | |||
|
|
|||
| [flake8] | |||
| max-line-length = 90 | |||
There was a problem hiding this comment.
this seems unrelated but ok
There was a problem hiding this comment.
Yes, not related to the issue, but with 79 chars I can't even put the link on the source of CachedProperty without splitting it. That's silly.
| @@ -22,6 +22,18 @@ class DataURIError(ValueError): | |||
| pass | |||
|
|
|||
|
|
|||
| # https://github.com/bottlepy/bottle/commit/fa7733e075da0d790d809aa3d2f53071897e6f76 | |||
| class CachedProperty(object): | |||
There was a problem hiding this comment.
nit: cached_property (or an alias cached_property = CachedProperty) makes usage look more like @property for which this is a drop-in replacement anyway
|
in general, looks good. changelog entry and docs + example in README would be needed as well |
Oh, right. Sorry. Fixed 👍 |
| def text(self): | ||
| if not self.media_type.startswith('text/'): | ||
| return None | ||
| return self.data.decode(self.charset or 'ascii') |
There was a problem hiding this comment.
this will raise LookupError in case the encoding is a weird name like foo. should we error out or return None in that case?
There was a problem hiding this comment.
I think, something should be raised to allow lib users to make the decision about such cases themself.
close #3
close #6