atom + utf-8 support. #5
Conversation
|
Thanks for the pull request, I had a look and I'll give you a review before merging it |
|
|
||
| def tostring(self, nsmap: Optional[Dict[str, str]] = None): | ||
| nsmap = nsmap or {} | ||
| nsmap = nsmap or {'atom': 'http://www.w3.org/2005/Atom'} |
There was a problem hiding this comment.
Do not default to atom mode. The library must be kept consistent as is (i.e. compliant with the original RSS specification).
In order to ease the implementation I am not opposed to dropping the nsmap kwarg and adding an atom / podcast kwarg to generate the nsmap accordingly.
| @@ -46,7 +46,7 @@ def _generate_tree_list(root: etree.Element, key: str, value: List[dict]) -> Non | |||
| if content is not None: | |||
| itemroot.text = content | |||
| else: | |||
| RSSFeed.generate_tree(itemroot, item) | |||
| RSSFeed.generate_tree(itemroot, item, {}) | |||
There was a problem hiding this comment.
This change won't be necessary (see other comments)
| RSSFeed.generate_tree(channel, self.dict()) | ||
| return etree.tostring(rss, pretty_print=True, xml_declaration=True) | ||
| RSSFeed.generate_tree(channel, self.dict(), nsmap) | ||
| return etree.tostring(rss, pretty_print=True, xml_declaration=True, encoding='UTF-8') |
There was a problem hiding this comment.
I should have originally encoded the string as utf-8, I agree, but the original RSS specification didn't mention a specific encoding to use (to be fair, at the time of the spec for RSS 2.0, UTF-8 wasn't widely supported yet).
Encoding as UTF-8 can be the default, but it should be able to be opted out if need be, in order to keep the library consistent with its previous version.
Furthermore, such a change will warrant a major version bump. Please upgrade the version in pyproject.toml to reflect that.
| @@ -8,7 +8,7 @@ def test_rss_sample_response(client: TestClient, expected_response: str, capsys) | |||
| response = client.get('/1') | |||
| assert response.status_code == 200 | |||
| tree = etree.fromstring(response.content) | |||
| pretty: str = etree.tostring(tree, pretty_print=True, xml_declaration=True).decode('ascii') | |||
| pretty: str = etree.tostring(tree, pretty_print=True, xml_declaration=True).decode('utf-8') | |||
There was a problem hiding this comment.
With my other comment, this change is not needed anymore, instead make a new test specifically for the utf8 encoded version.
No description provided.