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

Fix CustomMessageListener class, and update example and documentation #2208

Closed

Conversation

samuelspagl
Copy link
Contributor

This is an implementation of the changes mentioned in #2195 which therefore can be closed.

Content of the PR

In previous commits there was the CustomMessageListener class implemented, to add type hinting to the message listeners.
This PR is fixing the bug of not being able to utilise this typehinting class. It makes sure to enable users to use the @CustomMessageListener decorator for their listener functions, while still being backwards compatible.

Changes made

In runners.py

class CustomMessageListener(Protocol):
    @abstractmethod
    def __call__(self, environment: "Environment", msg: Message) -> None:

to

class CustomMessageListener:
    def __init__(self, f):
        self.f = f  # Store the function

    def __call__(self, environment: "Environment", msg: Message) -> None:
        self.f(environment=environment, msg=msg)

Additional Changes:

Updated the custom_messages.py example and the running-distributed.rst documentation file.

@samuelspagl
Copy link
Contributor Author

ping @nathan-beam, @cyberw

@cyberw
Copy link
Collaborator

cyberw commented Sep 19, 2022

Looking at the updated example I'm not sure about this. This is different from other Locust event handlers work, and I'm not sure if the added complexity (importing a class, tagging the method) is worth it. But lets have Nathan weigh in.

@samuelspagl
Copy link
Contributor Author

Okay :) then let's wait.

Just to be on the same page: it is fully backwards compatible. So you can also just use it without the decorator. It's just for the type consistency.

So the other side would be completely deleting the typehint?

For me it was very confusing to have this not working as expected type hint :)

@cyberw
Copy link
Collaborator

cyberw commented Sep 19, 2022

Right, we shouldnt have incorrect type hints either, so thats not an option :)

@samuelspagl
Copy link
Contributor Author

So any news about this? :) Just wanna check.

@cyberw
Copy link
Collaborator

cyberw commented Oct 8, 2022

I guess @nathan-beam is asleep. I dont like having to introduce a decorator to the examples. There should be some more user friendly solution. But I’m afk, and probably wont really have the energy once I get back :)

@samuelspagl
Copy link
Contributor Author

Okay, so rn I don't have another solution for this. I honestly think that the decorator is kinda the cleanest method, as it also is not breaking any existing code.

Then I would propose to remove the CustomMessageListener class completely. That way it will be type consistent again.
I can prepare a PR, if you want that. :)

@cyberw
Copy link
Collaborator

cyberw commented Oct 12, 2022

I think that might be a good idea. That class never really made sense to me :) If you make a PR I’ll be sure to review it.

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

Successfully merging this pull request may close these issues.

2 participants