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

IGNITE-15102 Add handling events. #46

Closed
wants to merge 6 commits into from

Conversation

ivandasch
Copy link
Contributor

No description provided.

Copy link
Contributor

@isapego isapego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but there are few questions inline.

"""
from pyignite import AioClient
from pyignite.api.cluster import cluster_get_state_async, cluster_set_state_async

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file even been there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you have added this by mistake and I've missed this on review. I realised that this is file is not used at all while ran test with coverage.

pass


class _EventListeners:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it common practice in Python that there is a separate method for every event? Why won't we just add a event_type field to an event and on_event message to listener? With current approach we need to create new methods for every new event.

Copy link
Contributor Author

@ivandasch ivandasch Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With current approach we need to create new methods for every new event.

Yep, but it is much more convenient to user. Firstly, user should not create these methods by hand. Secondly,
it should not write a bunch if..elif..else code. Plus, python is a dynamic-typed language and there is not much help from IDE when you code such a thing. In current apporach these problems are eliminated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the examples of PyCharm IDE help in this case:

  1. Add new event handler
    Peek 2021-07-19 18-18
  2. Autocomplete fields of event
    Peek 2021-07-19 18-24

Copy link
Contributor

@isapego isapego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, ship it

@ivandasch ivandasch closed this in 82f29e2 Jul 20, 2021
ivandasch added a commit to Sberbank-Technology/ignite-python-thin-client that referenced this pull request Jul 20, 2021
ivandasch added a commit that referenced this pull request Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants