Bug 222770 – Adding Python type annotations to test_expectations.py

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sam Sneddon [:gsnedders]]

This file has all kinds of confusing code flow, and I have no faith in my ability to refactor it without breaking everything in subtle ways.

This adds PEP 484-with-Python-2-support annotations to the file, which suffice for mypy --follow-imports skip --strict webkitpy/layout_tests/models/test_expectations.py to succeed without errors. I'd much rather be using the Python 3-only syntax, but that's not an option. (Not rushing to add annotations to the rest of webkitpy till Python 2 support is gone, unless I end up similarly stuck.)

This is based on automatically generated annotations from https://github.com/dropbox/pyannotate running test_expectations_unittest.py, albeit with a fair bit of work to get everything passing with Mypy.

[Sam Sneddon [:gsnedders]]

Created attachment 422313 [details]
Patch

[Jonathan Bedard]

Comment on attachment 422313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422313&action=review

I think we should land this without integrating Mypy into check-webkit-style, but if we intend to use more type annotations in webkitpy, we should integrate the type checker into check-webkit-style

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:47
> +    from typing import (

How will we ever run this? Line 44 sets MYPY to false, is that overridden when running Mypy?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:450
> +        s = self.to_string(None)

I would have done this like so:

return self.to_string(None) or ''

But more to the point, we usually try to use `if not s` unless we need an explicit None check, and I don't think we do need that explicit check here.

[Sam Sneddon [:gsnedders]]

(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 422313 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422313&action=review
> 
> I think we should land this without integrating Mypy into
> check-webkit-style, but if we intend to use more type annotations in
> webkitpy, we should integrate the type checker into check-webkit-style

I agree.

> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:47
> > +    from typing import (
> 
> How will we ever run this? Line 44 sets MYPY to false, is that overridden
> when running Mypy?

MYPY automatically sets MYPY=True, so yes. What would clarify the comment?

> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:450
> > +        s = self.to_string(None)
> 
> I would have done this like so:
> 
> return self.to_string(None) or ''

Yeah, I guess especially in the string case this isn't worth the perf cost.

> But more to the point, we usually try to use `if not s` unless we need an
> explicit None check, and I don't think we do need that explicit check here.

I guess I'm too used to trying to avoid __bool__ calls for None-checks!

[Jonathan Bedard]

Comment on attachment 422313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422313&action=review

>>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:47
>>> +    from typing import (
>> 
>> How will we ever run this? Line 44 sets MYPY to false, is that overridden when running Mypy?
> 
> MYPY automatically sets MYPY=True, so yes. What would clarify the comment?

Comment is fine, just weird that MYPY works that way. Not sure there is much to be done, this will hopefully die soon when Python 2 does.

[EWS]

Committed r274220: <https://commits.webkit.org/r274220>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422313 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75272854>