Critic review: https://critic.hoppipolla.co.uk/r/1443

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

I should mention that my branch of pip has the same change proposed here in its vendor lib inclusion of html5lib

Really I'd rather Jython just changed so that is matched the Python documentation and CPython and PyPy and had its unicode type be 16-/32-bit code-units. :)

But…

Trying to run the tests with that branch of Jython on your branch of html5lib leads to a bunch of errors. Notably, html5lib/tests/test_tokenizer.py reports an error after two tests, when it's trying to run tests for lone surrogates in the input stream. I suppose this should just be a documented limitation of html5lib on Jython, and some workaround added so the rest of the tokenizer tests run to completion.

Otherwise there's a bunch of failures due to lack of an euc_jp codec. The docs say:

Python comes with a number of codecs built-in, either implemented as C functions or with dictionaries as mapping tables. The following table lists the codecs by name, together with a few common aliases, and the languages for which the encoding is likely used.

…which I take to mean as Python, as a language, comes with them. Hence that's seems to be another bug in Jython.

Guido van Rossum summarized the issue of UTF-16 support in Jython well, which we used as guidance when Jython 2.5 was under development:
https://mail.python.org/pipermail/python-3000/2006-September/003821.html

This has all been rehashed endlessly. It's implementation (and
platform- and compilation options-) dependent because there are good
reasons for both choices. Even if CPython 3.0 supports a dynamic
choice (which some are proposing) then the language will still make
it implementation dependent because of Jython and IronPython, where
the only choice is UTF-16 (or UCS-2, depending the attitude towards
surrogates).

Re euc_jp codec, this is a known bug with respect to a lack of CJK codecs (http://bugs.jython.org/issue1066). Hopefully there has been some more recent work on the corresponding patch.

However, I did just re-run using

$ ~/jythondev/jython-socket-reboot/dist/bin/nosetests --verbose
...
----------------------------------------------------------------------
Ran 1038 tests in 16.498s

OK

Maybe I need to specify additional options?

Did you init/update the git submodule? See the readme. You should be looking at tens of thousands of tests.

FWIW, while the Py2 language reference uses the ambiguous phrasing of "Unicode ordinal" in defining the unicode type (what on earth a "Unicode ordinal" is is a very good question — it's not defined anywhere!), the Py3 language reference states that the str type is a sequence of Unicode codepoints (which would therefore mean lone surrogates are allowed). But this is rehashing what I said in the Jython bug and probably not the place to debate Python semantics.

@gsnedders Got it, I was able to run 16729 tests, so we are now on the same page. Of these I'm seeing 7 errors. 6 are definitely euc_jp; 1 is failing in the middle of a loop, which I haven't investigated yet. Maybe this is euc_jp too?

Since we would like to fix http://bugs.jython.org/issue1066 and support CJK codecs, I'm going to put this PR on hold on our side until that CJK support is complete, which I would be expect in the beta 4. This timing seems to be sound given the work by Yuji Yamano and the fact that this is now getting some attention. Because we will also will be integrating ensurepip during beta 4, there may be an interval where ensurepip will refer to a Jython-specific fork of pip (so beyond the current practice of referring people to https://github.com/jimbaker/pip to try things out), but this should be short in duration.

The other error is another case of lone-surrogates, I can tell you that much.

@gsnedders re the other case: will take a look. Thanks again for your help!

I have updated http://bugs.jython.org/issue1066 to reflect this CJK codec dependency.

When you next take a look at this, mind adding $py.class to .gitignore?

So where are we with this right now?

The CJK work is now complete in Jython trunk: Jython now supports being able to wrap java.nio.charset codec support with the standard Python codec API. This includes the tested euc_jp codec in this test suite.

There's one remaining problem with isolated surrogates with the tests corresponding to unicodeCharsProblematic.test (a data file used to construct distinct unit tests, a rather clever approach); some sort of workaround will be required.

I will update the branch with this test workaround accordingly, along with .gitignore.

All tests pass with python 2.7, python 3.4, and jython 2.7 trunk using html5lib-tests test data.

With this latest change, Jython will now only run 17353 of the current 17357 tests, effectively excluding the following test cases in unicodeCharsProblematic.test:

{"tests" : [
{"description": "Invalid Unicode character U+DFFF",
"doubleEscaped":true,
"input": "\\uDFFF",
"output":["ParseError", ["Character", "\\uFFFD"]]},

{"description": "Invalid Unicode character U+D800",
"doubleEscaped":true,
"input": "\\uD800",
"output":["ParseError", ["Character", "\\uFFFD"]]},

{"description": "Invalid Unicode character U+DFFF with valid preceding character",
"doubleEscaped":true,
"input": "a\\uDFFF",
"output":["ParseError", ["Character", "a\\uFFFD"]]},

{"description": "Invalid Unicode character U+D800 with valid following character",
"doubleEscaped":true,
"input": "\\uD800a",
"output":["ParseError", ["Character", "\\uFFFDa"]]},

However, it does run the test case in that data file using \u0000.

I did notice that the JSON parsing of namedentities.test is rather slow. Something to fixed separately, by using the Jackson package for the json module implementation.

I should also mention that https://bitbucket.org/jimbaker/jython-socket-reboot has been deleted, now that it has been merged into https://bitbucket.org/jython/jython (or alternatively, http://hg.python.org/jython)

For what it's worth, if this gets merged and released within the next month or so It'll make it into pip 1.6. Not sure what the release schedule looks like for html5lib though :)

@gsnedders Any updates/comments on this pull request? We are really hoping that pip 1.6 will be able to support Jython 2.7, as well as other users of html5lib-python

Apologies for being a bit slow (I've been all over the place travelling pretty continuously, and I'm now on holiday till EuroPython); there's a couple of comments on Critic now, see the first comment above.

@dstufft: the html5lib release schedule is typically someone going, "oh, hey, I need this bugfix that's on master, could you make a release?", and me responding with a release. :) (In theory, I plan on making releases every so often once anything worthwhile happens, but other people often find obscure bugfixes worthwhile, so it gets released when they ask.)

@gsnedders Looking forward to seeing you at EuroPython! I'll respond to the Critic review, thanks!

I added some more comments on critic.

Hmm, test failures are due to trailing whitespace.

#168 is essentially the same as this with the trailing whitespace fixed.

#168 fails all its tests, not just the flake8 ones.

That odd given nonameentername@c4755d3 is the only extra commit on the branch… Literally the whole diff is just that one commit! So, um… Somehow submodule not being updated properly on Travis? Because that looks like it's run with a more recent html5lib-tests revision…

Oh, Travis CI seems to run the implicit merge-commit in html5lib-python@refs/pull/168/merge. Hence given it's based on master (which currently fails tests because #174 isn't fixed yet) and run more recently it fails.

(That's annoying because it means that when Travis CI runs a given PR it tests different things; if I were to make it retest #150 it would have all the same failures, but with flake8 finding nothing, le sigh.)

I was hoping that the recent jython 2.7b4 release somehow worked around this problem, but it is still borken.

Hmm, I don't know of any resolution here. Are you in the process of merging #168?

Just closing this because #168 is it's replacement rather than keeping both open.

(We'll see if anything happens soon. Maybe.)