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

added __enter__ and __exit__ methods #4078

Closed
wants to merge 6 commits into from
Closed

added __enter__ and __exit__ methods #4078

wants to merge 6 commits into from

Conversation

zamai
Copy link

@zamai zamai commented May 25, 2017

Adding enter and exit methods to the WebDriver class to create support for the with function in python
https://docs.python.org/3/reference/compound_stmts.html#with

Adding __enter__ and __exit__ methods to the WebDriver class to create support for the with function in python
 https://docs.python.org/3/reference/compound_stmts.html#with
@davehunt
Copy link
Contributor

Why not add these to the base class, so all sub-classes inherit it? Also, could you add tests? I know this has been proposed before, and although I have no objection to it, I wonder if there's a reason not to add it that I'm unaware of. @AutomatedTester?

@@ -61,6 +61,15 @@ def __init__(self, executable_path="phantomjs",
raise

self._is_remote = False
"""
In order for the python with statement to work __enter__ and __exit__ methods must be deffined in the object.
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this comment is needed. if anything, I would just state "enter and exit methods enable using this class as a context manager"

Copy link
Author

Choose a reason for hiding this comment

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

Removed comment.

@zamai
Copy link
Author

zamai commented Jun 4, 2017

@davehunt I've added the methods to the Class level.
But I'm not sure regarding the tests, how exactly should I test this?
Thanks,

@davehunt
Copy link
Contributor

davehunt commented Jun 5, 2017

But I'm not sure regarding the tests, how exactly should I test this?

Add a test that using with to exercise this change. I'm also keen to hear from @AutomatedTester if he's comfortable with this change.

@AutomatedTester
Copy link
Member

I dont mind this change.

@p0deje p0deje added the C-py label Jun 7, 2017
@lmtierney
Copy link
Member

@zamai are you still planning on writing tests for this?

@zamai
Copy link
Author

zamai commented Oct 26, 2017

@lmtierney hi, I'm planning to write them this weekend, will let you know here

@cgoldberg
Copy link
Contributor

@zamai, small nitpicks:
The linter (flake8) is failing on your branch. Could you fix the formatting? You should add 2 blank lines before the __enter__ method and remove the trailing whitespace from the code inside both methods.
thanks!

@zamai
Copy link
Author

zamai commented Oct 29, 2017

Hey, @cgoldberg and @lmtierney
I've updated the PR.
Now there are not flake8 errors, from what I see
Thanks for helping me with it

@cgoldberg
Copy link
Contributor

cgoldberg commented Oct 29, 2017

lgtm, but still needs tests

@lmtierney
Copy link
Member

@zamai This still needs a simple test, please update

@lmtierney
Copy link
Member

Closed in favor of #5919

@lmtierney lmtierney closed this May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants