8000 Support for log_event and get_events command by ki4070ma · Pull Request #469 · appium/python-client · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@ki4070ma
Copy link
Collaborator
@ki4070ma ki4070ma commented Nov 9, 2019

@ki4070ma ki4070ma changed the title Use appium/events as endpoint to get events [WIP] Use appium/events as endpoint to get events Nov 9, 2019
@ki4070ma ki4070ma changed the title [WIP] Use appium/events as endpoint to get events Support for log_event and event command Nov 10, 2019
@mykola-mokhnach
Copy link
Contributor

cc @dpgraham

Can you please take a look at this change? I remember you were working on #429

@mykola-mokhnach
Copy link
Contributor

I am ok with the PR, although I would like @dpgraham to check it before merging, since it might be a breaking one

@ki4070ma ki4070ma requested a review from dpgraham November 10, 2019 12:17
class LogEvent(webdriver.Remote):

@property
def events(self):
Copy link
Member
@KazuCocoa KazuCocoa Nov 10, 2019

Choose a reason for hiding this comment

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

https://github.com/appium/appium-base-driver/blob/master/lib/protocol/routes.js#L589-L594
/events route accepts type argument as option.
It hasn't been implemented yet, but this events should not be property.

You can rename here as get_events(self, type = None) to keep another events as session_capability['events']. It is for backword compatibility as #469 (comment)

Copy link
Collaborator Author
@ki4070ma ki4070ma Nov 10, 2019

Choose a reason for hiding this comment

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

@KazuCocoa
Thanks, I misunderstood, I will keep original events and create get_events as another one.

@ki4070ma ki4070ma changed the title Support for log_event and event command Support for log_event and get_events command Nov 10, 2019
It isn't implemented yet for now
startTime: (int) Received time
endTime: (init) Response time
"""
return self.execute(Command.GET_EVENTS)['value']
Copy link
Member

Choose a reason for hiding this comment

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

As a client, below two cases are available

self.execute(Command.LOG_EVENT, {})
self.execute(Command.LOG_EVENT, {'type': nil}) (<= Appium server will handle this 'type' to filter events)

Copy link
Collaborator Author
@ki4070ma ki4070ma Nov 10, 2019

Choose a reason for hiding this comment

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

Yes, I've checked and added, but removed by below commit since it isn't implemented yet.
In my opinion, it's enough to add type when it's ready.

0a84069

However, I'll restore type along to your comments. Any other comments are welcome.

Copy link
Member

Choose a reason for hiding this comment

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

It could depend on server-side if the parameter works.
We can handle the behaviour only server-side modification (and documentation on appium.io), if clients follow the route definition like other commands.

The value isn't passed to the server for now.
@KazuCocoa
Copy link
Member

Added type.
Please check the description in appium/appium-base-driver#368 about the available format

@ki4070ma ki4070ma requested a review from KazuCocoa November 16, 2019 15:09
@ki4070ma ki4070ma merged commit b797254 into appium:master Nov 18, 2019
@ki4070ma ki4070ma deleted the fix-events branch November 18, 2019 14:51
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.

3 participants

0