Skip to content

Remove incomplete thrift stubs that cause false positives. #1827

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

Merged
merged 1 commit into from
Jan 26, 2018

Conversation

carljm
Copy link
Member

@carljm carljm commented Jan 13, 2018

Per the precedent set in #1659 and #1664, I am removing the thrift stubs. As noted in 84d0cbf, "These stubs are pure stubgen and might not be much good." In fact, they are not much good, and we saw false positives from them when upgrading mypy (because of their recent move from 2 to 2and3, which made them visible to us for the first time). The false positives we saw are due to classes which are missing entirely from the stubs; comparing the stubs to the actual source of Apache thrift, they are woefully incomplete.

IMO these should be in their own project, used via PEP 561.

@carljm
Copy link
Member Author

carljm commented Jan 13, 2018

Side note: it seems that incomplete typeshed stubs are rather hard to work around, other than by sprinkling type: ignore all over the place. I tried stuff like this in our mypy.ini to no avail:

[mypy-thrift.*]
ignore_errors=True
follow_imports=skip

Not sure why these config options seem to be ineffective for typeshed stubs.

@gvanrossum
Copy link
Member

Can you create a separate project to host the stubs you are deleting, and accepting improvements? That's what's been done consistently with other stubs we deleted. Since Thrift (IIUC) comes out of Facebook this could well be a Facebook GitHub project.

IIRC the other problem you mention (hard to work around missing stubs) is specific to packages whose top-level folder is in typeshed but with some specific submodules missing. Can you look for a mypy bug tracking that, or file one if you can't find one?

facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Jan 18, 2018
Summary:
D6738525 and D6731821 (python/typeshed#1827)
broke type checking on Eden's Python.  This works around removing the thrift
stubs.

Reviewed By: carljm

Differential Revision: D6745233

fbshipit-source-id: 760f7ee199698a0a570a993f16b45c833910aade
@JelleZijlstra
Copy link
Member

These stubs are sufficiently stubby that I'm OK with just removing them now. Starting a new thrift-stubs project from scratch wouldn't be significantly more work than starting from scratch. @gvanrossum any objections to merging this now?

@gvanrossum
Copy link
Member

gvanrossum commented Jan 26, 2018 via email

@carljm
Copy link
Member Author

carljm commented Jan 26, 2018

I've started things rolling on FB-hosted thrift stubs, it makes sense for us to own them. But merging this PR now doesn't make that any harder :-)

Couldn't find an existing issue for the ignore-missing-imports problem; filed python/mypy#4515

rhysparry added a commit to rhysparry/typeshed that referenced this pull request Feb 8, 2018
* python/master: (269 commits)
  allow Optional[float] for asyncio.wait() timeout arg (python#1860)
  Clean up the pytype blacklist. (python#1851)
  filter function: make the callable of the first overload non-optional so that mypy is able to select the second overload for the  case. (python#1855)
  Accept Text in distutils *Version classes (python#1854)
  Add attrs library (python#1838)
  Add type stub for the lzma module (python#1844)
  Add ImportError attributes name, path for Python 3.3+ (python#1849)
  Add 'message' to click.decorators.version_option (python#1845)
  PEP 553 and PEP 564 (python#1846)
  Adding py36 additions to datetime module (python#1848)
  Remove incomplete thrift stubs that cause false positives. (python#1827)
  adding curses.ascii, curses.panel and curses.textpad modules (python#1792)
  Fix requests session hooks type (python#1794)
  Add StringIO.name and BytesIO.name (python#1802)
  Fix werkzeug environ type (python#1831)
  Change the return type of unittest.TestCase.fail() to NoReturn (python#1843)
  _curses.tparm works on bytes, not str (python#1828)
  Refine types for distutils.version (python#1835)
  Add stub for stat.filemode (python#1837)
  modify pytype_test to run from within pytype too, and support python3 (python#1817)
  ...
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.

3 participants