Skip to content

Conversation

@whiten
Copy link
Contributor

@whiten whiten commented Feb 18, 2020

This includes many small changes to make mypy understand why various
constructs are safe, and in a few cases to fix real bugs. Many of mypy's
resolved complaints had to do with functions that sometimes return None
(e.g., inspect.currentframe()), and immediate dereferences of these
"union" types made of the expected return type and None.

While making these changes, Python 2-isms were in some cases removed if
they made fixing the problematic statements simpler to fix or the
resulting code easier to understand.

This also introduces scripts/flake8 to allow a minor monkeypatch of
pyflakes (part of flake8) to incorporate
PyCQA/pyflakes@fa92556,
allowing for "# type: ignore[attr-defined]" comments allowing for
specific ignore of Mypy complaints. With this, fixes all flake8
complaints in our codebase.

This was committed to pyflakes in August but there have been no releases
and we shouldn't be holding our breath for one per issues like
PyCQA/pyflakes#475.

@whiten whiten added the WIP label Feb 18, 2020
@whiten whiten requested a review from a team as a code owner February 18, 2020 06:58
@whiten whiten force-pushed the mypy2 branch 2 times, most recently from 12d11e4 to fbc0303 Compare February 18, 2020 17:27
@whiten whiten requested a review from kantuni February 19, 2020 00:51
@whiten whiten removed the WIP label Feb 19, 2020
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the reason behind as here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In lib/mypy.ini I set implicit_export = false (https://mypy.readthedocs.io/en/stable/config_file.html#miscellaneous-strictness-flags), which is documented as By default, imported values to a module are treated as exported and mypy allows other modules to import them. When false, mypy will not re-export unless the item is imported using from-as or is included in __all__.

Example apps were failing mypy checking by using st.cache, and this fixed that, while still having us fail if someone tried to use st.subprocess. We probably should define __all__ in the future to make our interface explicit, but in the short term should I leave this change in, or instead see if I can change the value of implicit_export for this module only?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm OK with leaving it this way for now. Just add a comment explaining the weird as.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kantuni, added comment.

This includes many small changes to make mypy understand why various
constructs are safe, and in a few cases to fix real bugs. Many of mypy's
resolved complaints had to do with functions that sometimes return None
(e.g., inspect.currentframe()), and immediate dereferences of these
"union" types made of the expected return type and None.

While making these changes, Python 2-isms were in some cases removed if
they made fixing the problematic statements simpler to fix or the
resulting code easier to understand.

This also introduces scripts/flake8 to allow a minor monkeypatch of
pyflakes (part of flake8) to incorporate
PyCQA/pyflakes@fa92556,
allowing for "# type: ignore[attr-defined]" comments allowing for
specific ignore of Mypy complaints. With this, fixes *all* flake8
complaints in our codebase.

This was committed to pyflakes in August but there have been no releases
and we shouldn't be holding our breath for one per issues like
PyCQA/pyflakes#475.
self._session = self._server._create_report_session(self)

def on_close(self):
if not self._session:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kantuni, in this and the next function, added some early bailouts soself._session is guaranteed to be non-None in the function body.

@whiten whiten merged commit b3bcd7d into streamlit:develop Feb 20, 2020
@whiten whiten deleted the mypy2 branch February 20, 2020 00:25
tconkling added a commit to tconkling/streamlit that referenced this pull request Feb 20, 2020
* develop:
  [mypy] Enable check_untyped_defs throughout the codebase. (streamlit#1110)
tconkling added a commit to tconkling/streamlit that referenced this pull request Feb 24, 2020
* develop: (53 commits)
  Update plotly.js to 1.52 (streamlit#1119)
  Add tooltip in Altair/Vega-lite docstring code (streamlit#1092)
  Ability to bind to a server address with server.address config option (streamlit#1107)
  [mypy] Enable check_untyped_defs throughout the codebase. (streamlit#1110)
  st.map: set "radiusMinPixels" to 3 (streamlit#1113)
  st.plotly_chart docs incorrectly refer to Altair (streamlit#1118)
  Fix BytesIO and numpy array data sources for audio/video (streamlit#1116)
  Show warning to user and use skipkeys=True if json.dumps causes TypeError (streamlit#1112)
  Server: store SessionInfo by id (streamlit#1056)
  [Screencast] Fixing countdown bug (streamlit#1082)
  Release 0.56.0 (streamlit#1101)
  Fix bug where icon assets were missing from "make install" (streamlit#1100)
  Clean signatures of wrapped DeltaGenerator methods (streamlit#1099)
  Using widgetId as the key when rendering elements (streamlit#1102)
  Pass --skip-lock to pipenv under pipenv-install Makefile target. (streamlit#1093)
  Allow wider range of int-like types in NumberInput (streamlit#1087)
  Fix missing caching URL (streamlit#1094)
  Version 0.55.1 (just pointing to new docs) (streamlit#1089)
  Retire python-3.7.4 CircleCI project. (streamlit#1086)
  Handle different Caching error messages (streamlit#1068)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants