py: Implement dict for class #2843

touilleMan · 2017-02-06T23:37:33Z

Based on #1757 PR

dpgeorge · 2017-02-07T05:56:05Z

Thanks for the PR.

Please can you give some background on why you needed this feature?

Also, it would be much simpler to just return self->locals_dict directly, instead of making a copy.

touilleMan · 2017-02-07T08:59:40Z

I'm using MicroPython to provide Python as a script language for the Godot game engine.

Basically Godot engine works with a gui where you drop nodes, which you can connect to a script class.

from godot import exposed, export
from godot.bindings import Node2D

@exposed
class Player(Node2D):
    name = export(str, default=”Samuel”)

    def _ready(self):
        pass

Loading this module will result from the Godot point of view of making available the Player class, which says the extended node should provide a name field in the gui editor for the user to customize this value.

To implement this, I should iterate over all the class fields and retrieve the ones which are instance of export.
It's quite possible I'll end up doing this at C level anyway, but given this is a (C)Python feature and it is already available for the class instances, I guess it's good to improve MicroPython compatibility anyway :-)

Also, it would be much simpler to just return self->locals_dict directly, instead of making a copy.

That's what instance's __dict__ implementation is doing (it is stated this way in the faq about the differences with CPython).

By the way I fixed my patch to support the case where locals_dict is defined to NULL.

pfalcon · 2017-02-07T09:23:31Z

Thanks for the detailed description of your usecase, it's really helpful to know how people use MicroPython! And thanks for selecting MicroPython as your embedding engine, that's one of the area we'd like to have more traction, but lack resources. But we'd really like people selecting us as their Python implementation not because we're doing it like CPython, but because we're doing it differently (because otherwise, using CPython is the most obvious way, and we won't be able to compete with CPython even if we could, but we don't have any agenda like that). This is described in more detail in https://github.com/micropython/micropython/wiki/ContributorGuidelines , and should be insightful read.

To implement this, I should iterate over all the class fields and retrieve the ones which are instance of export.

You can, without any patching and fully compatibly with CPython:

class Foo:

    a = 1
    b = 2.0

    def meth(self):
        pass


print(dir(Foo))
for f in dir(Foo):
    val = getattr(Foo, f)
    print(f, val, type(val))

touilleMan · 2017-02-07T21:28:26Z

And thanks for selecting MicroPython as your embedding engine

Thanks for writting MicroPython then 😃

You can, without any patching and fully compatibly with CPython:

Problem is using dir will get me not only the class attributes but also it parents' (in my usecase parents' exported attribute are already provided by the inheritance so I don't want to shadow them and mess the binding)

touilleMan · 2017-02-10T21:46:08Z

btw I don't understand why travis flag the build is considered as failed given the tests seems to have passed...

stinos · 2017-02-11T08:43:39Z

Check the build log in Travis:

../py/objtype.c: In function ‘type_attr’:

../py/objtype.c:833:35: error: comparison between pointer and integer [-Werror]

             if (self->locals_dict != MP_OBJ_NULL) {

                                   ^

../py/objtype.c:834:17: error: passing argument 1 of ‘mp_obj_dict_get_map’ makes integer from pointer without a cast [-Werror]

                 mp_map_t *map = mp_obj_dict_get_map(self->locals_dict);

                 ^

In file included from ../py/objtype.h:29:0,

                 from ../py/objtype.c:34:

../py/obj.h:741:11: note: expected ‘mp_obj_t’ but argument is of type ‘struct _mp_obj_dict_t *’

 mp_map_t *mp_obj_dict_get_map(mp_obj_t self_in);

           ^

cc1: all warnings being treated as errors

make[1]: *** [build-nanbox/py/objtype.o] Error 1

make[1]: Leaving directory `/home/travis/build/micropython/micropython/unix'

make: *** [nanbox] Error 2

make: Leaving directory `/home/travis/build/micropython/micropython/unix'

The command "make -C unix nanbox" exited with 2.

touilleMan · 2017-02-11T09:40:34Z

@stinos thanks for the help !

Now everything is fine 😄

dpgeorge · 2017-02-13T01:54:09Z

py/objtype.c

+                        mp_obj_dict_store(attr_dict, map->table[i].key, map->table[i].value);
+                    }
+                }
+            }


Why can't you simply do attr_dict = self->locals_dict (as long as locals_dict is not NULL)? It's not 100% the same as CPython because you can edit the returned dict and it'll modify the class, but it's much simpler.

The idea was to stay close to #1757 code, beside not copying locals_dict means you can modify it only if it is not null, which is pretty strange from the Python point of view (e.g. sometime you can modify __dict__, sometime the modification is not taken into account...)
Anyway I've simplify my implementation to follow you're advice. I you prefer the first solution in the end, just tell me and I'll revert ;-)

touilleMan · 2017-02-26T21:56:12Z

@dpgeorge Updated following your advices

pfalcon · 2017-05-03T21:46:44Z

Let me be straight: I'm -1 on this. And the reason is right there - in the submitter's own words:

Problem is using dir will get me not only the class attributes but also it parents' (in my usecase parents' exported attribute are already provided by the inheritance so I don't want to shadow them and mess the binding)

So, how __dict__ works is actually an implementation detail. Relying that it will provide only own class attributes is not well grounded. Actually, even relying that it exists is not very grounded, and MicroPython makes that very clear by not providing. One exciting area for future uPy development is making it much more efficient by making it much more static. Then there will be no any __dict__'s at all. We aren't yet there because we're fully in grunt work of closing the accidental gaps in functionality we have. But if in teh meantime we'll add features going against the future exciting things we could do with uPy, we'll simply never get there. So again, -1 (for this year, can revisit next one).

dpgeorge · 2021-05-04T04:14:41Z

This was implemented in 28370c0

py: Implement __dict__ for class

c6c9e65

py: Fix __dict__ for C class without locals_attr field

6247e51

py: Fix cast error with __dict__ for C class on picky compiler

2551e99

dpgeorge reviewed Feb 13, 2017

View reviewed changes

py: Simplify implementation of __dict__ for class

5fb42ef

pfalcon added the contrib-not-for-mainline label May 14, 2017

dpgeorge closed this May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

py: Implement dict for class #2843

py: Implement dict for class #2843

py: Implement __dict__ for class #2843

py: Implement __dict__ for class #2843

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

py: Implement dict for class #2843

py: Implement dict for class #2843