Fix mypy errors #229

johananl · 2019-10-21T09:48:44Z

63f559e seems to have broken mypy since we are returning a literal False in 63f559e#diff-97345450e765fa40d32664356325e06cR248 while using typing.Optional[bool] as the type hint.

This causes the following mypy error:

opentelemetry-api/src/opentelemetry/trace/__init__.py:243: error: "bool" is invalid as return type for "__exit__" that always returns False
opentelemetry-api/src/opentelemetry/trace/__init__.py:243: note: Use "typing_extensions.Literal[False]" as the return type or change it to "None"
opentelemetry-api/src/opentelemetry/trace/__init__.py:243: note: If return type of "__exit__" implies that it may return True, the context manager may swallow exceptions
opentelemetry-api/src/opentelemetry/trace/__init__.py:243: error: "bool" is invalid as return type for "__exit__" that always returns False
opentelemetry-api/src/opentelemetry/trace/__init__.py:243: note: Use "typing_extensions.Literal[False]" as the return type or change it to "None"
opentelemetry-api/src/opentelemetry/trace/__init__.py:243: note: If return type of "__exit__" implies that it may return True, the context manager may swallow exceptions

~~I don't know why the CI was green on that commit, but anyway it is failing now.~~ As it turns out we don't pin the mypy version so apparently this started breaking with mypy 0.740.

AFAICT this can be solved by changing the type hint to typing_extensions.Literal[False], however reading the docs it seems the only return value which suppresses exceptions is True, therefore dropping the return statement altogether should have the same result as returning a literal False and would also allow us to set the type hint to -> None and fix mypy.

Returning a literal causes a mypy error when combined with the `typing.Optional[bool]` type hint. Furthermore, exception handling is the same when returning `False` and when returning `None` (the exception is re-raised). Therefore, it's simpler to remove the return statement and change the type hint to `None`.

johananl · 2019-10-21T09:54:47Z

Looks like there is another mypy error "hiding" behind this one. I'm looking into it.

Oberon00 · 2019-10-21T10:01:19Z

I wonder why this wasn't detected in the PR build. Maybe the CI is using different mypy versions on master vs branch builds?

Oberon00 · 2019-10-21T10:05:08Z

Yes, that seems to be a new requirement since mypy 0.740, see http://mypy-lang.blogspot.com/2019/10/mypy-0740-released.html The rationale seems to be that the Literal[false] allows mypy to detect that a with block using that context manager will never swallow exceptions.

I think we had some discussion about pinning tool/dependency versions but decided against it because we'd rather "stay up to date" and fix such errors.

Tuples of length 1 should be initialized with a trailing comma to be properly interpreted.

mauriciovasquezbernal · 2019-10-21T11:23:02Z

Duplicated of #227?

Oberon00 · 2019-10-21T11:24:14Z

Thank you for battling mypy here and fixing bugs along the way! I think at this point, running mypy locally tox -e mypy would be faster as new errors seem to turn up all the time.

EDIT: Probably running tox -r -e mypy once to have an up-to-date mypy would be best.

johananl · 2019-10-21T11:28:29Z

Duplicated of #227?

Oops, thanks @mauriciovasquezbernal. In the meantime I've fixed all the errors in this PR, too. I've tried to solve the problems mypy is complaining about rather than ignoring them. Which option is the preferred one?

johananl · 2019-10-21T11:29:36Z

I think at this point, running mypy locally tox -e mypy would be faster as new errors seem to turn up all the time.

Of course, I do that normally. In this case the changes in the newer mypy version threw me off so I didn't trust my local setup anymore :-)

Since we have `disallow_untyped_calls = True` in our mypy config for tests, we must add type annotations to any function that is called from a test.

Oberon00 · 2019-10-21T11:40:26Z

Whoops, I didn't see #227 either. I'd say, since @johananl made the effort to fix this without type: ignore, I'd prefer this PR.
Additionally, we should probably bump our minimum mypy version in the tox.ini to 0.740.

johananl · 2019-10-21T11:42:24Z

Additionally, we should probably bump our minimum mypy version in the tox.ini to 0.740.

Done.

mauriciovasquezbernal

LGTM!

carlosalberto · 2019-10-21T14:55:46Z

Hey, thanks for looking into this - I had myself ran tox -e mypy locally and nothing was reported as wrong ;(

Oberon00 · 2019-10-21T15:26:22Z

I'll go ahead and merge this since it is likely uncontroversial and makes our master and all PR builds red.

@carlosalberto: I think using -r (recreate) with tox would catch that, but I'm not entirely sure. But at least with bbab148, one should be able to reproduce similar problems locally now.

Oberon00 · 2019-10-21T15:35:21Z

Aaand, master is green again! 💚

In particular, the following errors are fixed in this commit: * Don't return False in __exit__ Returning a literal causes a mypy error when combined with the `typing.Optional[bool]` type hint. Furthermore, exception handling is the same when returning `False` and when returning `None` (the exception is re-raised). Therefore, it's simpler to remove the return statement and change the type hint to `None`. * Correctly initialize nested tuple Tuples of length 1 should be initialized with a trailing comma to be properly interpreted. * Pass correct type to use_context() in test * Add type annotations for test helper functions Since we have `disallow_untyped_calls = True` in our mypy config for tests, we must add type annotations to any function that is called from a test. Addditionally, bump minimal mypy version to 0.740 to consistently reproduce these errors.

* feat: add plugin config options * fix: resolve conflicts * fix: review comments

johananl requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners October 21, 2019 09:48

Correctly initialize nested tuple

d4d1333

Tuples of length 1 should be initialized with a trailing comma to be properly interpreted.

johananl added 2 commits October 21, 2019 13:32

Pass correct type to use_context() in test

88f428c

Add type annotations for test helper functions

8dd4160

Since we have `disallow_untyped_calls = True` in our mypy config for tests, we must add type annotations to any function that is called from a test.

johananl force-pushed the johananl/fix-mypy branch from 6dc1057 to 8dd4160 Compare October 21, 2019 11:33

Bump minimal mypy version to 0.740

bbab148

johananl changed the title ~~Don't return False in __exit__~~ Fix mypy errors Oct 21, 2019

Oberon00 approved these changes Oct 21, 2019

View reviewed changes

mauriciovasquezbernal mentioned this pull request Oct 21, 2019

fix mypy type annotations #227

Closed

mauriciovasquezbernal approved these changes Oct 21, 2019

View reviewed changes

Oberon00 mentioned this pull request Oct 21, 2019

Check Iterator[Span] return type annotations #219

Closed

carlosalberto approved these changes Oct 21, 2019

View reviewed changes

Oberon00 merged commit 3fa3a83 into open-telemetry:master Oct 21, 2019

johananl deleted the johananl/fix-mypy branch October 21, 2019 15:41

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

feat: add plugin config options (open-telemetry#229)

8279133

* feat: add plugin config options * fix: resolve conflicts * fix: review comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mypy errors #229

Fix mypy errors #229

johananl commented Oct 21, 2019 •

edited

Loading

johananl commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

mauriciovasquezbernal commented Oct 21, 2019

Oberon00 commented Oct 21, 2019 •

edited

Loading

johananl commented Oct 21, 2019

johananl commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

johananl commented Oct 21, 2019

mauriciovasquezbernal left a comment •

edited

Loading

carlosalberto commented Oct 21, 2019 •

edited

Loading

Oberon00 commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

Fix mypy errors #229

Fix mypy errors #229

Conversation

johananl commented Oct 21, 2019 • edited Loading

johananl commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

mauriciovasquezbernal commented Oct 21, 2019

Oberon00 commented Oct 21, 2019 • edited Loading

johananl commented Oct 21, 2019

johananl commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

johananl commented Oct 21, 2019

mauriciovasquezbernal left a comment • edited Loading

Choose a reason for hiding this comment

carlosalberto commented Oct 21, 2019 • edited Loading

Oberon00 commented Oct 21, 2019

Oberon00 commented Oct 21, 2019

johananl commented Oct 21, 2019 •

edited

Loading

Oberon00 commented Oct 21, 2019 •

edited

Loading

mauriciovasquezbernal left a comment •

edited

Loading

carlosalberto commented Oct 21, 2019 •

edited

Loading