mojo: Generate top level enums for python bindings.

mojo: Generate top level enums for python bindings.

R=pkl@chromium.org,cmasone@chromium.org

Committed: https://crrev.com/c03bfd9f1a1e7a48dc2f3498b4e4d653aaf109d5
Cr-Commit-Position: refs/heads/master@{#294144}

[qsr, 2014-09-04 15:17:21]

CC: +tburkard

[Chris Masone, 2014-09-04 17:20:32]

https://chromiumcodereview.appspot.com/545433002/diff/100001/mojo/public/pyth...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://chromiumcodereview.appspot.com/545433002/diff/100001/mojo/public/pyth...
mojo/public/python/mojo/bindings/reflection.py:33: if isinstance(key, str) and
isinstance(enum_value, int):
This latter isinstance() check is what you asked the list about, right? You'll
need to allow enum_value to itself be an instance of the type you're defining,
right?

[pkl (ping after 24h if needed), 2014-09-04 18:07:20]

lgtm

https://chromiumcodereview.appspot.com/545433002/diff/80001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://chromiumcodereview.appspot.com/545433002/diff/80001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:34: class_members[key],
current_enum_value = enum_value, enum_value + 1
This is going to silently overwrite previously defined enums. Do you want to
have error check on class_members.has_key(key) before you do this assignment?

[qsr, 2014-09-05 11:46:01]

qsr@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[qsr, 2014-09-05 11:46:02]

+viettrungluu@ -> Trung -> Can you take a look at the changed in the mojom pylib
and confirm that you are fine with the change:

Mainly it give access to the enum in an EnumValue instead of just the name, and
also check that an enum value can be resolved correctly, and do not just stays a
pair ('IDENTIFIER', str)

https://codereview.chromium.org/545433002/diff/80001/mojo/public/python/mojo/...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://codereview.chromium.org/545433002/diff/80001/mojo/public/python/mojo/...
mojo/public/python/mojo/bindings/reflection.py:34: class_members[key],
current_enum_value = enum_value, enum_value + 1
On 2014/09/04 18:07:20, pklpkl wrote:
> This is going to silently overwrite previously defined enums. Do you want to
> have error check on class_members.has_key(key) before you do this assignment?

 Not really, if that happens, that must be caught at a higher level, not by each
individual code generators.

https://codereview.chromium.org/545433002/diff/100001/mojo/public/python/mojo...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://codereview.chromium.org/545433002/diff/100001/mojo/public/python/mojo...
mojo/public/python/mojo/bindings/reflection.py:33: if isinstance(key, str) and
isinstance(enum_value, int):
On 2014/09/04 17:20:32, Chris Masone wrote:
> This latter isinstance() check is what you asked the list about, right? You'll
> need to allow enum_value to itself be an instance of the type you're defining,
> right?

 That was the reason of my message, yes. See the latest version: I now
pre-compute all enum values.

[viettrungluu, 2014-09-05 17:05:30]

Hrm, we really should rewrite/refactor everything in pylib/mojom/generate/. It's
an opaque mess (with dubious error-checking and handling).

https://codereview.chromium.org/545433002/diff/140001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/data.py (right):

https://codereview.chromium.org/545433002/diff/140001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/data.py:106: raise
Exception('Unable to resolve expression: %r' % value[1])
Does this properly handle, e.g., using constants as default values to fields?

[viettrungluu, 2014-09-05 17:05:47]

On 2014/09/05 17:05:30, viettrungluu wrote:
> Hrm, we really should rewrite/refactor everything in pylib/mojom/generate/.
It's
> an opaque mess (with dubious error-checking and handling).

(I'm not implying that you need to do this.)

> 
>
https://codereview.chromium.org/545433002/diff/140001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/pylib/mojom/generate/data.py (right):
> 
>
https://codereview.chromium.org/545433002/diff/140001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/pylib/mojom/generate/data.py:106: raise
> Exception('Unable to resolve expression: %r' % value[1])
> Does this properly handle, e.g., using constants as default values to fields?

[qsr, 2014-09-08 09:06:09]

https://codereview.chromium.org/545433002/diff/140001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/data.py (right):

https://codereview.chromium.org/545433002/diff/140001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/data.py:106: raise
Exception('Unable to resolve expression: %r' % value[1])
On 2014/09/05 17:05:30, viettrungluu wrote:
> Does this properly handle, e.g., using constants as default values to fields?

 Yes, we do have this in our test cases
(mojo/public/interfaces/bindings/tests/sample_service.mojom). What it prevents
us to do is used a constant as a value for an enum. If we want to support this,
we will need to have LookupValue returns the right NamedValue here instead of
letting the  tuple untouched.

[qsr, 2014-09-09 08:00:21]

Chris, Trung, gentle ping? pkl is not a commiter yet.

[qsr, 2014-09-09 08:19:33]

qsr@chromium.org changed reviewers:
+ sdefresne@chromium.org

[qsr, 2014-09-09 08:19:59]

+sdefresne

[sdefresne, 2014-09-09 08:46:10]

lgtm

https://codereview.chromium.org/545433002/diff/140001/mojo/public/python/mojo...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://codereview.chromium.org/545433002/diff/140001/mojo/public/python/mojo...
mojo/public/python/mojo/bindings/reflection.py:23: '__new__': None,
nit: you probably want to keep __module__ defined here to what is in dictionary.

[viettrungluu, 2014-09-09 22:12:07]

lgtm

[qsr, 2014-09-10 07:23:49]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-10 07:27:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/545433002/140001

[commit-bot: I haz the power, 2014-09-10 08:23:05]

Committed patchset #8 (id:140001) as 405af2f62c2629187f64e5b98fd73b01f0b54da9

[qsr, 2014-09-10 09:01:09]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-10 09:01:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/545433002/140001

[commit-bot: I haz the power, 2014-09-10 09:02:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 09:02:17]

Failed to apply patch for mojo/public/python/mojo/bindings/__init__.py:
While running git apply --index -p1;
  error: mojo/public/python/mojo/bindings/__init__.py: already exists in index

Patch:   NR 
tools/usb_gadget/__init__.py->mojo/public/python/mojo/bindings/__init__.py
Index: mojo/public/python/mojo/bindings/__init__.py
diff --git a/tools/usb_gadget/__init__.py
b/mojo/public/python/mojo/bindings/__init__.py
similarity index 100%
copy from tools/usb_gadget/__init__.py
copy to mojo/public/python/mojo/bindings/__init__.py

[commit-bot: I haz the power, 2014-09-10 10:56:07]

Patchset 8 (id:??) landed as
https://crrev.com/c03bfd9f1a1e7a48dc2f3498b4e4d653aaf109d5
Cr-Commit-Position: refs/heads/master@{#294144}