Be consistent with webdriver init kwarg service_log_path #5979
Conversation
Deprecate log_file for IE and log_path for Edge and Firefox. Fixes SeleniumHQ#5725
| if ie_options: | ||
| warnings.warn('use options instead of ie_options', DeprecationWarning) | ||
| options = ie_options | ||
| self.port = port | ||
| if self.port == 0: | ||
| self.port = utils.free_port() | ||
| self.host = host | ||
| self.log_level = log_level | ||
| self.log_file = log_file |
There was a problem hiding this comment.
Is there a reason to spare log_level and log_file instance vars?
There was a problem hiding this comment.
I don't see a code relevance as to why they are included as instance vars other than the fact that you can check them after setting them since they aren't available on the Service
There was a problem hiding this comment.
But you're right, I don't see them in any other webdrivers.
|
@Dude-x care to have a look? |
|
What about Chrome? |
|
Chrome already uses |
|
LGTM. |
|
Right, we should try and get this merged in the next release |
|
Merged, thank you for your contribution! |
| capabilities=None, port=0, verbose=False, log_path=None): | ||
| capabilities=None, port=0, verbose=False, service_log_path=None, log_path=None): | ||
| if log_path: | ||
| warnings.warn('use service_log_path instead of log_path', DeprecationWarning) |
There was a problem hiding this comment.
Adding a stacklevel here and in the similar instances below, would help make the resultant warnings clearer, since they would reference the offending caller rather than the .warn() line in these files. I've filed #6274 for this (but it would be good to watch out for this when reviewing future PRs).
…umHQ#5979) Deprecate log_file for IE and log_path for Edge and Firefox. Fixes SeleniumHQ#5725
Deprecate log_file for IE and log_path for Edge and Firefox.
Fixes #5725
Xin the preceding checkbox, I verify that I have signed the Contributor License Agreement