[py] PEP 484 type hints for common.exceptions and webdriver.support.color by hoefling · Pull Request #9482 · SeleniumHQ/selenium

hoefling · 2021-05-17T21:17:56Z

Description

This is the initial PR that adds type hints to Python codebase. Typed modules:

selenium.common.exceptions,
webdriver.support.color.
I have selected those two for the initial typing since they do not depend on any other modules and can be typed in isolation.

Motivation and Context

The goal of this PR is to configure and add initial integration for the mypy tool (checks the type hints validity). It can be run via

$ tox -e mypy

or

$ TOXENV=mypy tox

Right now it emits 1248 errors, mainly due to missing type hints; the goal is to gradually reduce it to zero. No errors are emitted in modules typed in this PR.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

CLAassistant · 2021-05-17T21:18:04Z

All committers have signed the CLA.

hoefling · 2021-05-17T21:35:26Z

@AutomatedTester gentle ping (no rush though!). Please advise:

Is the PR too big for a review? I will split it if necessary and create smaller PRs in the future.
I would like to add a new job that runs mypy checks, but it will fail until all the codebase is typed and unfortunately GH actions syntax doesn't allow to ignore failing jobs. I can filter out untyped modules in mypy config (so no errors are reported) and loosen the filter gradually with each typed module, like suggested in mypy docs:
```
# mypy config
[mypy]
...
[mypy-selenium.webdriver.*]
ignore_errors = True

[mypy-selenium.webdriver.support.event_firing_webdriver]
ignore_errors = True

# etc
```
However, maybe you have a better idea for me?

AutomatedTester

@AutomatedTester gentle ping (no rush though!). Please advise:
1. Is the PR too big for a review? I will split it if necessary and create smaller PRs in the future.

No, this is fine. Don't go much bigger than this though.

2. I would like to add a new job that runs `mypy` checks, but it will fail until all the codebase is typed and unfortunately GH actions syntax doesn't allow to ignore failing jobs. I can filter out untyped modules in `mypy` config (so no errors are reported) and loosen the filter gradually with each typed module, like suggested in [mypy docs](https://mypy.readthedocs.io/en/stable/existing_code.html):
   ```ini
   # mypy config
   [mypy]
   ...
   [mypy-selenium.webdriver.*]
   ignore_errors = True
   
   [mypy-selenium.webdriver.support.event_firing_webdriver]
   ignore_errors = True
   
   # etc
   ```

I've not used mypy in anger so I might need your help here. Is there a way that we can do a pass over and have file that holds the current output so we can't regress and then fix the others? A little similar to how Rubocop works for Ruby. If your suggestion above does that then do it and we can then clean it up as we go. Let's try not boil the ocean.

AutomatedTester · 2021-05-18T13:47:21Z

py/selenium/common/exceptions.py

    """

-    def __init__(self, msg=None, screen=None, stacktrace=None):
+    def __init__(self, msg: Any = None, screen: Any = None, stacktrace: Any = None) -> None:


These are always going to be str or `None

AutomatedTester · 2021-05-18T13:58:04Z

py/selenium/common/exceptions.py

    commands.
    """
-    def __init__(self, msg=None, screen=None, stacktrace=None, alert_text=None):
+    def __init__(self, msg: Any = None, screen: Any = None, stacktrace: Any = None, alert_text: Any = None) -> None:


The types here are str or None.

@AutomatedTester I guess except stacktrace? Looking at this right now:

selenium/py/selenium/webdriver/remote/errorhandler.py

Lines 208 to 236 in c138008

stacktrace = None

st_value = value.get('stackTrace') or value.get('stacktrace')

if st_value:

if isinstance(st_value, str):

stacktrace = st_value.split('\n')

else:

stacktrace = []

try:

for frame in st_value:

line = self._value_or_default(frame, 'lineNumber', '')

file = self._value_or_default(frame, 'fileName', '<anonymous>')

if line:

file = "%s:%s" % (file, line)

meth = self._value_or_default(frame, 'methodName', '<anonymous>')

if 'className' in frame:

meth = "%s.%s" % (frame['className'], meth)

msg = " at %s (%s)"

msg = msg % (meth, file)

stacktrace.append(msg)

except TypeError:

pass

if exception_class == UnexpectedAlertPresentException:

alert_text = None

if 'data' in value:

alert_text = value['data'].get('text')

elif 'alert' in value:

alert_text = value['alert'].get('text')

raise exception_class(message, screen, stacktrace, alert_text)

raise exception_class(message, screen, stacktrace)

hoefling · 2021-05-20T18:43:13Z

I've not used mypy in anger so I might need your help here. Is there a way that we can do a pass over and have file that holds the current output so we can't regress and then fix the others? A little similar to how Rubocop works for Ruby. If your suggestion above does that then do it and we can then clean it up as we go. Let's try not boil the ocean.

I see what you mean - unfortunately, my suggestion doesn't do that, it's merely "hide modules with errors until those are fixed, and don't forget to uncover them back once fixed". However, mypy can produce JUnit and Cobertura reports. Let me play with them - I think the report file can be cached on main branch and compared against the report in the PR. Maybe we can even integrate that in a service like codecov.io and get an automatic regression reports in PRs. However, I wouldn't bloat this PR up anymore and introduce the CI job in another PR.

Signed-off-by: oleg.hoefling <[email protected]>

sonarqubecloud · 2021-05-27T13:39:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

AutomatedTester reviewed May 18, 2021

View reviewed changes

hoefling requested a review from AutomatedTester May 23, 2021 13:27

hoefling added 5 commits May 23, 2021 15:29

add mypy configuration, tox env to run mypy checks

cdb8c38

Signed-off-by: oleg.hoefling <[email protected]>

add type hints for selenium.common.exceptions

9bb8bdc

Signed-off-by: oleg.hoefling <[email protected]>

add type hints for selenium.webdriver.support.color

3a69d52

Signed-off-by: oleg.hoefling <[email protected]>

restrict exception args types to string or None

4ccc8f4

Signed-off-by: oleg.hoefling <[email protected]>

change type of stacktrace argument to a string sequence

6ed6f8a

Signed-off-by: oleg.hoefling <[email protected]>

hoefling force-pushed the pep-484-type-hints branch from 8f331c7 to 6ed6f8a Compare May 23, 2021 13:29

AutomatedTester approved these changes May 27, 2021

View reviewed changes

Merge branch 'trunk' into pep-484-type-hints

7542f9e

AutomatedTester merged commit f75343f into SeleniumHQ:trunk May 27, 2021

hoefling deleted the pep-484-type-hints branch May 27, 2021 18:37

This was referenced May 27, 2021

[py] Run type checks in CI and report typing coverage to codecov #9523

Merged

[py] PEP 484 type hints for selenium.webdriver.common.timeouts #9524

Merged

[py] PEP 484 type hints for selenium.webdriver.remote.utils #9525

Merged

hoefling mentioned this pull request Jun 13, 2021

[py] PEP 484 type hints for selenium.webdriver.common.utils #9568

Merged

8 tasks

This was referenced Jun 27, 2021

[py] PEP 484 type hints for selenium.webdriver.remote.errorhandler #9605

Merged

[py] PEP 484 type hints for selenium.webdriver.remote.file_detector #9606

Merged

[py] PEP 484 type hints for selenium.webdriver.common.print_page_options #9608

Merged

This was referenced Nov 28, 2021

[py] PEP 484 type hints for selenium.webdriver.chromium.options #10099

Merged

[py] PEP 484 type hints for selenium.webdriver.common.actions.wheel_input #10103

Merged

[py] PEP484 hints for webdriver.common.actions.action_builder #10108

Merged

iampopovich mentioned this pull request Dec 26, 2021

[py] PEP484 hints for py/selenium/webdriver/common/actions/key_input.py #10186

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[py] PEP 484 type hints for common.exceptions and webdriver.support.color#9482

[py] PEP 484 type hints for common.exceptions and webdriver.support.color#9482
AutomatedTester merged 6 commits intoSeleniumHQ:trunkfrom
hoefling:pep-484-type-hints

hoefling commented May 17, 2021 •

edited

Loading

Uh oh!

CLAassistant commented May 17, 2021 •

edited

Loading

Uh oh!

hoefling commented May 17, 2021

Uh oh!

AutomatedTester left a comment

Uh oh!

AutomatedTester May 18, 2021

Uh oh!

AutomatedTester May 18, 2021

Uh oh!

hoefling May 23, 2021

Uh oh!

hoefling commented May 20, 2021

Uh oh!

sonarqubecloud bot commented May 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	stacktrace = None
	st_value = value.get('stackTrace') or value.get('stacktrace')
	if st_value:
	if isinstance(st_value, str):
	stacktrace = st_value.split('\n')
	else:
	stacktrace = []
	try:
	for frame in st_value:
	line = self._value_or_default(frame, 'lineNumber', '')
	file = self._value_or_default(frame, 'fileName', '<anonymous>')
	if line:
	file = "%s:%s" % (file, line)
	meth = self._value_or_default(frame, 'methodName', '<anonymous>')
	if 'className' in frame:
	meth = "%s.%s" % (frame['className'], meth)
	msg = " at %s (%s)"
	msg = msg % (meth, file)
	stacktrace.append(msg)
	except TypeError:
	pass
	if exception_class == UnexpectedAlertPresentException:
	alert_text = None
	if 'data' in value:
	alert_text = value['data'].get('text')
	elif 'alert' in value:
	alert_text = value['alert'].get('text')
	raise exception_class(message, screen, stacktrace, alert_text)
	raise exception_class(message, screen, stacktrace)

Uh oh!

Conversation

hoefling commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

Checklist

Uh oh!

CLAassistant commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoefling commented May 17, 2021

Uh oh!

AutomatedTester left a comment

Choose a reason for hiding this comment

Uh oh!

AutomatedTester May 18, 2021

Choose a reason for hiding this comment

Uh oh!

AutomatedTester May 18, 2021

Choose a reason for hiding this comment

Uh oh!

hoefling May 23, 2021

Choose a reason for hiding this comment

Uh oh!

hoefling commented May 20, 2021

Uh oh!

sonarqubecloud bot commented May 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hoefling commented May 17, 2021 •

edited

Loading

CLAassistant commented May 17, 2021 •

edited

Loading