Feature/python3 support #40
Conversation
robszewczyk
requested review from
gerickson,
jaylogue and
mrjerryjohns
as code owners
In python 3 comparison between different classes is not defined by default; fix the Jinja template to sort on a common attribute type instead. Comparison between int and None is not defined; define suitable fallback
`sys.stdin.read()` and `sys.stdout.write()` methods in Python3 expect to operate on strings, and thus expect to read/write valid unicode objects. The protoc python interface expects to be able to send/receive aribitrary binary streams. The change here keeps the existing sys.stdin and sys.stdout objects on Python2 systems and on Python3 systems, the code uses raw binary IO via `open()` call.
Python3 does not permit dictionaries to be modified during iteration. Replace the philosophy of deleting the object from a dictionary with maintaining a list of already completed objects.
In python 3, the result of `struct.pack` is already a `bytes` object; simply map it to a list for compatibility purposes. Keep prior mapping via `ord` for cases where `struct.pack` returns a `str` (i.e. Python2)
| @@ -53,7 +53,7 @@ enum {{ enum.base_name }} { | |||
| chain(get_struct_dependencies(obj)|reject('common')|map(attribute='parent'))| | |||
| chain(get_struct_dependencies(obj)|select('common'))| | |||
| reject('false')|unique|reject('standard')|reject('vendor')| | |||
| reject('equalto',obj)|sort -%} | |||
| reject('equalto',obj)|sort(attribute='full_name') -%} | |||
There was a problem hiding this comment.
This seems unrelated to the Python 3 upgrade. Is it?
There was a problem hiding this comment.
It is VERY much related. In Python2, comparison between unrelated objects would yield a stable result, in Python3, in the absence of a comparison operator, this throws an error. In this case, the objects sorted were derived from a common ancestor, but defined no comparison. When we were comparing Typespace and Trait, the comparison would throw an error. I would note that the new code parallels the structure we've seen in other templates for codegen that have not been opensourced yet.
| @@ -3288,7 +3300,7 @@ if ${nl_cv_protoc_version+:} false; then : | |||
| $as_echo_n "(cached) " >&6 | |||
| else | |||
|
|
|||
| nl_cv_protoc_version=`${PROTOC} --version 2>&1 | ${SED} -n -e 's/^.\{1,\}\([0-9]\{1,\}.[0-9]\{1,\}.[0-9]\{1,\}\)$/\1/gp'` | |||
| nl_cv_protoc_version=`${PROTOC} --version 2>&1 | ${SED} -n -E -e 's/^[^0-9]*(([0-9]+\.)*[0-9]+).*/\1/p'` | |||
There was a problem hiding this comment.
@gerickson you cool with this regex change?
There was a problem hiding this comment.
This should be auto-generated and should match what came in via Eli Ribble from commit ea8339e
There was a problem hiding this comment.
Note: this regex change has been previously approved and pulled into configure.ac in #39 ; in this pull request we simply update the automake-generated files. The change was made to support protoc-3.6.1 and similar style of versions
robszewczyk
force-pushed
the
feature/python3-support
branch
from
a94e66b to
23898b8
Compare
No description provided.