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

Event based callback signatures #15

Open
mnesarco opened this issue Dec 8, 2024 · 12 comments
Open

Event based callback signatures #15

mnesarco opened this issue Dec 8, 2024 · 12 comments
Labels
api-change API Changes

Comments

@mnesarco
Copy link
Owner

mnesarco commented Dec 8, 2024

          @galou 

I will introduce a different approach here to make it more flexible so i don't have to change the signatures again in the future, I want to do this change once and only once. So I will introduce Event classes everywhere and all signatures will receive a specific Event and no more arguments. this way Event classes can be enhanced over time without breaking existing code.

Example (pseudocode):

class PropertyChangeEvent[T]:
  old_value: T
  new_value: T
  target: DocumentObject


@my_prop.observer
def listener3(self, event: PropertyChangeEvent):
  print(f"my_property3 has changed to {event.new_value}")

I will introduce the same approach for everything on_execute, on_init, ....

This also makes signatures easier to remember.

Originally posted by @mnesarco in #2 (comment)

@galou
Copy link
Contributor

galou commented Dec 9, 2024

I cannot assess whether this will render the API more developer-friendly. It's probably a good idea but it appears to me this may not be necessary. The signature for on_execute is (self), this cannot be easier to remember. Anyway, you're on the best position to assess and I like the fact that there's no redundancy anymore.

@mnesarco
Copy link
Owner Author

mnesarco commented Dec 9, 2024

@galou The main goal is to make the API stable, so data passed to the api methods can change over time without breaking existing code. The evolution of FreeCAD API is unpredictable that is why I don't use inheritance (that would be easier) but metaprogramming to adapt to the changes internally while maintains a stable and documented api exposed to the extension developers. I think this change is worth to do now that not too many people uses fcapi, then when more and more extensions depend on it, it will be harder and harder to introduce changes.

With this change, all methods can be implemented with one of two signatures, for example

def on_execute(self):
  # Valid
  
def on_execute(self, event):
  # Valid
  
def on_execute(self, event: ExecuteEvent):
  # Valid
  

@mnesarco
Copy link
Owner Author

mnesarco commented Dec 9, 2024

CC @luzpaz I don't know how familiar are you with current fcapi but you have been following my previous scripting related work. Do you want to comment something about this planned change?

@mnesarco
Copy link
Owner Author

mnesarco commented Dec 9, 2024

CC @tomate44
Hello Chris, as you are using fcapi now, maybe you want to comment on this topic.

@tomate44
Copy link

tomate44 commented Dec 9, 2024

Hi,
I have only started using fcapi a few days ago, and I am far less skilled than you, so I fully trust you and will adapt to your decision.

@galou
Copy link
Contributor

galou commented Dec 9, 2024

With the flexible signature it makes a lot of sense.

@luzpaz
Copy link

luzpaz commented Dec 10, 2024

Thanks @mnesarco but unfortunately this is out of my purview. Maybe worth running by wwmayer ?

@mnesarco
Copy link
Owner Author

I believe the change is worth to do. I will push it to the devel branch first.

@galou
Copy link
Contributor

galou commented Dec 11, 2024

Nice, let met know when I can try out.

@mnesarco
Copy link
Owner Author

@galou you can download the file from the devel branch to try it out:
https://github.com/mnesarco/fcapi/blob/devel/fpo.py

but documentation is still outdated.

@mnesarco
Copy link
Owner Author

Hello Friends, Any comments about the proposed changes in the devel branch? I plan to go with it this week and update docs...

@galou
Copy link
Contributor

galou commented Dec 17, 2024

Sorry, I didn't have the time to test it out.

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

No branches or pull requests

4 participants