Python3 support #48
Python3 support #48
Conversation
There was a problem hiding this comment.
Thanks for working on this!
The individual changes look good. Couple of minor comments inline.
But big comment: it gives wrong results for non-ascii message strings. I tried it on Products.Poi (branch 2.x, call rebuild_i18n.sh) and it gives differences like this:
#. Default: "Tags"
#: ./skins/Poi/poi_issue_search_results.pt:41
msgid "listingheader_tags"
-msgstr "Značky"
+msgstr "Zna\u010dky"
...
#. Default: "Edit"
#: ./browser/response.pt:20
msgid "Edit"
-msgstr "编辑"
+msgstr "\u7f16\u8f91"
And when I set the Plone site to Chinese, I see those escaped codes on the page. Unfortunately, there is some non-ascii test coverage already before your change, but apparently not enough... :-(
So some work left to do. I don't have an immediate suggestion an what to change. I hope it requires only minor further changes, like maybe switching to io.BytesIO.
Maybe it helps to split this PR a bit, like starting with fixing print statements and absolute imports and maybe some other changes suggested by the sixer command line tool. Then only test it on Python 2.7 for starters. I am not asking you to do this, but if it helps you: go ahead.
CHANGES.rst
Outdated
|
|
||
| New features: | ||
|
|
||
| - *add item here* | ||
| - Support python 3.6, 2.5, 3.4, pypy and pypy3. |
There was a problem hiding this comment.
Typo: 2.5 should be 3.5. :-)
setup.py
Outdated
|
|
||
| install_requires = [ | ||
| 'zope.tal >= 3.5.2', | ||
| 'zope.tal >= 4.3.0', | ||
| 'zope.interface >= 3.3', | ||
| 'zope.i18nmessageid >= 3.3', | ||
| 'ordereddict', |
There was a problem hiding this comment.
The ordereddict dependency can go away I think.
src/i18ndude/catalog.py
Outdated
| val.msgid = val.msgid.decode(self.encoding) | ||
| if getattr(val, 'msgstr', None) is not None: | ||
| if getattr(val, 'msgstr', None) is not None \ | ||
| and not isinstance(val.msgid, str): |
There was a problem hiding this comment.
Copy-paste error. This should be isinstance(val.msgstr. str).
There was a problem hiding this comment.
Fixed. Maybe this also fixes your reported bug?
|
O, when I ask the Screen shot of Plone UI in Chinese with the Poi locales done using your branch: |
|
I actually started with the various print statements, then later squashed my work to make review easier. What you see now is the first variant that passes tests on both python3.6 and python2.7. I'm personally actually less interested in python2.7 than I am in python3.6, so my sequence is to get it to work in python3.6 and then get it to work again in python2.7... I'll see if I can reproduce the non-ascii message strings in a test. I've caught various bugs around those already. I'll also address your other comments. |
|
@gyst I just enabled coveralls for i18ndude, would you mind updating travis configuration to also run coveralls? Question: should we merge tox.ini and .travis.yml ? I don't see one using the other, is already complex enough to have to maintain a system, let alone two... |
… was introduced in the stdlib.
|
@gforcada we should definitely not merge I'll try to see if your coveralls fix can be integrated into tox and travis. |
collective/i18ndude#48 but fails: There was an error processing Products/Poi/skins/Poi/poi-issue-search-rss.xml.pt Traceback (most recent call last): File "/var/tmp/eggs/i18ndude-4.0.1-py2.7.egg/i18ndude/extract.py", line 523, in tal_strings parser.parseFile(filename) File "/var/tmp/eggs/zope.tal-3.5.2-py2.7.egg/zope/tal/htmltalparser.py", line 122, in parseFile self.parseString(data) File "/var/tmp/eggs/zope.tal-3.5.2-py2.7.egg/zope/tal/htmltalparser.py", line 128, in parseString self.feed(data) File "/usr/lib/python2.7/HTMLParser.py", line 117, in feed self.goahead(0) File "/usr/lib/python2.7/HTMLParser.py", line 161, in goahead k = self.parse_starttag(i) File "/usr/lib/python2.7/HTMLParser.py", line 325, in parse_starttag self.handle_startendtag(tag, attrs) File "/var/tmp/eggs/zope.tal-3.5.2-py2.7.egg/zope/tal/htmltalparser.py", line 169, in handle_startendtag self.getpos()) TALError: empty HTML tags cannot use tal:content: 'link', at line 13, column 5, in file Products/Poi/skins/Poi/poi-issue-search-rss.xml.pt
|
I cannot reproduce your bug report @mauritsvanrees. I've checked out the I need I'm happy to fix any bugs in my work if I get a reproduction, ideally in the form of a test but a fully scripted build with a set of actions is OK too. But I'm not prepared to dive into a stack I don't really know, while having to guess what kind of toolchain and version ( |
|
@gforcada I don't use |
|
@gyst The The errors that you saw, are because the i18ndude html parser does not do well on templates that are meant for RSS. So the and this on master: The script expects the |
|
Ah, actually, the Same is true for With the |
|
Sorry, I can't reproduce this. It would be helpful to know what kind of environment/buildout you're using. I'm using If I run your commands I get: Sorry to be obnoxious. All I'm asking for is a reproducible set of steps that gives me a reproduction of the problem you're seeing, which I'm sure is real. Though, did you try and test whether the changes I pushed yesterday maybe fix them already? 593da2d |
|
You are not obnoxious. I am glad you are working on this! 👍 I did already try with your changes from yesterday. Let me try again. I am doing it like this:
Resulting script:
When you create a Plone Site with this language, you will see those ugly messages literally in the browser, so it really is a problem. When I use a Python 3.6.4 virtualenv, and try the same The And for a different error, call it on some Python files in the same directory: With Python 2.7 both |
|
OK that looks on first sight like something I will be able to reproduce, and the |
|
Hi @mauritsvanrees I managed to fix all the bugs you reported. If you read my inline comments you'll notice I have by now depleted my motivation to invest time in this package... I hope this is now mergeable and releasable. |
|
I can indeed imagine that you have had it by now... Thanks for sticking with it so far. Let me try. On Okay, when I do the same for the widgets, I get an error: Okay, I had a look at what current master did here. Master had no problems. But I did see that a manual search-and-replace was called for in these po files, which I did (collective/plone.app.locales@ce55662), and that luckily fixed the sync error with your branch too. Should be fixed before release, but I am fine with doing that in a separate issue. (Whether you or I or someone else picks that up, is another question.) The Overall: great job, thank you! I will merge. |
|
Note: I have created branch 4.x before merging this. |
|
Okay, I have fixed the remaining problem and have released 5.0.0. |
I'm porting
plone.recipe.codeanalysisto python3. This is a dependency that needed porting also.