-
Notifications
You must be signed in to change notification settings - Fork 572
Support for log_event and get_events command #469
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
Conversation
|
I am ok with the PR, although I would like @dpgraham to check it before merging, since it might be a breaking one |
| class LogEvent(webdriver.Remote): | ||
|
|
||
| @property | ||
| def events(self): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
It isn't implemented yet for now
| startTime: (int) Received time | ||
| endTime: (init) Response time | ||
| """ | ||
| return self.execute(Command.GET_EVENTS)['value'] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
However, I'll restore type along to your comments. Any other comments are welcome.
There was a problem hiding this comment.
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.
|
Added |
For appium/appium#13568