Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PyEcore has some unexpected (non-pythonic) behavior with EClass.behavior #80

Open
marvingreenberg opened this issue Dec 30, 2019 · 4 comments

Comments

@marvingreenberg
Copy link
Contributor

marvingreenberg commented Dec 30, 2019

Minor issue, and I'm unclear what the motivation for this design is - why not just have behavior be available on EClass, or just have the decorator be separate from EClass.

This issue arises from the code sample in doc/source/user/advanced.rst that says

import pyecore.behavior as behavior # We need to import the 'behavior' package
HelloWorld = EClass('HelloWorld')
@HelloWorld.behavior
def greeting(self):
     print("Hello")

But, knowing python, I knew that importing pyecore.behavior as some name (behavior) in the global namespace had nothing to do with behavior becoming available as a decorator on EClass.

$ python3
>>> from pyecore.ecore import EClass, EAttribute, EString
>>> [m for m in dir(EClass) if m.startswith('b')]
[]
>>> import pyecore.behavior as inattendu
>>> [m for m in dir(EClass) if m.startswith('b')]
['behavior']

It is unexpected, to me, for an import to add methods or attributes to some class in some other package. The whole point of this is apparently to add "behavior" to EClass, in order to allow it to be used as a decorator, like:

HelloWorld = EClass('HelloWorld')
@HelloWorld.behavior
def greeting(self):
    print('Hello World and', self.name)

But I looked at the code example, which said that "import pyecore.behavior" was needed, and knowing python thought to myself, that can't be true. Note that the "as behavior" is NOT needed, as my example changing it to "inattendu" demonstrated.

If you want a decorator for behavior, why not just have it. If it needs to have access to other information, such as some related class, just tell it. Then the code would become:

import pyecore.behavior as comportement
HelloWorld = EClass('HelloWorld')
@comportement(HelloWorld)
def greeting(self):
    print('Hello World and', self.name)

This would be completely equivalent:

import pyecore
HelloWorld = EClass('HelloWorld')
@pyecore.behavior(HelloWorld)
def greeting(self):
    print('Hello World and', self.name)

For some information on decorators with arguments, one reference is Decorators with arguments

@marvingreenberg
Copy link
Contributor Author

If you like I'll submit a pull-request when I have a chance changing this to a decorator with arguments. I looked at behavior.py, and the change looks simple enough (famous last words).

@marvingreenberg
Copy link
Contributor Author

marvingreenberg commented Dec 30, 2019

The dynamic case would then look like this fragment, if I understand things:

from pyecore.behavior import behavior as comportement
 <snip other code...>

hello = DynamicEPackage(mm_root)
@comportement(hello.HelloWorld)
def greeting(self):
    <snip>...

I also realize earlier I was confused - because you have the behavior module with the behavior decorator function within it. pyecore.behavior is the module, pyecore.behavior.behavior is the decorator function.

@marvingreenberg
Copy link
Contributor Author

Well, now I see the main and run decorators, which in your examples DO depend on the 'as behavior' import global name. This would be done consistently like this:

from pyecore.behavior import behavior, main, run
<snip...>
@main
@behavior(hello.HelloWorld)
def entry_point(self):
      self.greeting()

@aranega
Copy link
Member

aranega commented Feb 25, 2021

Hi @marvingreenberg

I took long enough to deal with this, but I added your proposition in behavior! Finally... I really think you make a point, I was trying to get an API "Flask-like", but it do not serve well here.
I kept the old-fashioned way of adding behaviors for retrocompatibility, but I will change this in the documentation later. The code is currently in develop.

Thanks again for the ticket and please accept my appologize for the delay of the answer/inclusion of your proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants