Create mypy_extensions package. by davidfstr · Pull Request #2228 · python/mypy

davidfstr · 2016-10-07T05:07:06Z

See discussion here: #2210

davidfstr · 2016-10-07T05:15:23Z

+    scripts=[],
+    data_files=mypy_setup.find_data_files('mypy_extensions', ['*.py', '*.pyi']),
+    classifiers=mypy_setup.setup_kwargs['classifiers'],
+)


This setup() call has all the same keys as the one in setup.py for mypy with one exception: There is no cmdclass key. I wasn't able to prove that it made sense here, so I did not include it.

davidfstr · 2016-10-07T05:17:10Z

+
+    new_dict.__name__ = typename  # type: ignore  # https://github.com/python/mypy/issues/708
+    new_dict.__supertype__ = dict  # type: ignore  # https://github.com/python/mypy/issues/708
+    return cast(Type[dict], new_dict)


If we don't want to add anything to mypy_extensions/typing.py initially, deferring to #2206 , we could blank it out.

I think it would be better to leave this empty for now.

davidfstr · 2016-10-07T05:18:40Z

@@ -1,18 +1,28 @@
 """Utilities for verifying git integrity."""

+from __future__ import print_function


This module must now support running in a Python 2.x context so that python setup.py in mypy_extensions works.

/mypy_extensions/setup.py --> /setup.py --> mypy/git.py

You need at least a comment reminding the reader. It would also be nice if the unit tests somehow enforced this. (If someone accidentally breaks this and the tests don't catch it, it's a real problem.)

But honestly I would much prefer if the setup.py for mypy_extensions didn't depend on this module at all.

davidfstr · 2016-10-07T05:24:33Z

+    packages=['mypy_extensions'],
+    scripts=[],
+    data_files=mypy_setup.find_data_files('mypy_extensions', ['*.py', '*.pyi']),
+    classifiers=mypy_setup.setup_kwargs['classifiers'],


It occurs to me that at least 'Programming Language :: Python :: 2' should probably be added to the classifiers for mypy_extensions.

Yeah, though I'm not sure whether anything actually checks those...

gvanrossum

Sorry for saying the same thing in seven different ways... I feel quite strongly about not complexificating setup files.

gvanrossum · 2016-10-10T21:17:56Z

@@ -0,0 +1,18 @@
+from typing import cast, Dict, Type, TypeVar


The more I think about the less I like that this module is named "typing.py". Yes, it's in a module namespace, but many people will probably prefer to write "from mypy_extensions import typing" and then use e.g. "typing.TypedDict", but if they also write "import typing" they have a name clash. And even if they don't it might confuse human readers to see "typing.TypedDict" (e.g. when using an IDE that hides the imports). Better not to invite confusion and give this a different name.

And honestly I also more and more believe that making this a package is over-engineering it. The cost of making it a package in the future are slight, and the probability that we'll never need that is high.

Also we need a module docstring and probably a comment explaining that cast..TypeVar are not intended to be exported.

Come to think of it, maybe we should just add a .pyi file adjacent with the signatures, and remove any type annotations from this file. In my experience with the actual top-level typing.py, I've found that the code implementing this doesn't benefit much from being type-checked itself, and there's a real cost (e.g. cast() is a pure-Python function and calling it is not particualrly cheap).

gvanrossum · 2016-10-10T21:18:10Z

@@ -0,0 +1,18 @@
+from typing import cast, Dict, Type, TypeVar
+
+


Don't need two blank lines here.

gvanrossum · 2016-10-10T21:23:59Z

+
+    new_dict.__name__ = typename  # type: ignore  # https://github.com/python/mypy/issues/708
+    new_dict.__supertype__ = dict  # type: ignore  # https://github.com/python/mypy/issues/708
+    return cast(Type[dict], new_dict)


I think it would be better to leave this empty for now.

gvanrossum · 2016-10-10T21:25:45Z

+import sys
+
+if '..' not in sys.path:
+    sys.path.insert(0, '..')


Have you tested this much with different installers (e.g. "python setup.py" as well as "pip install ")?

sys.path manipulations in a setup.py file give me the heebie-jeebies.

gvanrossum · 2016-10-10T21:26:42Z

+
+if '..' not in sys.path:
+    sys.path.insert(0, '..')
+import setup as mypy_setup


This (importing the top-level setup.py) also scares the living daylight out of me.

Most setup.py files are purely cargo-culted and work by accident.

gvanrossum · 2016-10-10T21:29:48Z

+    packages=['mypy_extensions'],
+    scripts=[],
+    data_files=mypy_setup.find_data_files('mypy_extensions', ['*.py', '*.pyi']),
+    classifiers=mypy_setup.setup_kwargs['classifiers'],


Yeah, though I'm not sure whether anything actually checks those...

gvanrossum · 2016-10-10T21:31:02Z

+    scripts=[],
+    data_files=mypy_setup.find_data_files('mypy_extensions', ['*.py', '*.pyi']),
+    classifiers=mypy_setup.setup_kwargs['classifiers'],
+)


gvanrossum · 2016-10-10T21:32:04Z

@@ -1,18 +1,28 @@
 """Utilities for verifying git integrity."""

+from __future__ import print_function


You need at least a comment reminding the reader. It would also be nice if the unit tests somehow enforced this. (If someone accidentally breaks this and the tests don't catch it, it's a real problem.)

But honestly I would much prefer if the setup.py for mypy_extensions didn't depend on this module at all.

gvanrossum · 2016-10-10T21:36:50Z

+# NOTE: Cannot use typing.TYPE_CHECKING guard here because cannot import
+#       "typing" in this file at all.
+if False:
+    from typing import Iterator


I prefer this:

MYPY = False if MYPY: from typing import Iterator

We'll have to support MYPY basically forever, while "if False" might eventually cause the body to be considered unreachable.

gvanrossum · 2016-10-10T21:39:51Z

+)
+
+if __name__ == '__main__':
+    setup(**setup_kwargs)


Oh, I don't like this at all. All this linkage between setup.py and extensions/setup.py is just extremely fragile.

davidfstr · 2016-10-11T03:58:47Z

I hear you loud and clear to be gentle with setup.py files. Here's a different approach:

Instead of having /mypy_extensions/setup.py depend directly on /setup.py, have both of them depend on a simpler shared module. Perhaps /mypy_extensions/setup_common.py . No more black magic with sys.path. No more if __name__ == '__main__' in setup.py, which may not even work.

Additionally mypy/git.py would be moved into this shared module since only the setup.py files use it. (There is an import in mypy/main.py but it is unused.) Can't remove the dependence entirely since the module version number is computed with git. Since mypy.git feels like an internal module (and therefore not part of the mypy API) I believe it should be safe to move without breaking external code.

And honestly I also more and more believe that making this a package is over-engineering it. The cost of making it a package in the future are slight, and the probability that we'll never need that is high.

Did you have another approach in mind for having experimental extensions to mypy? It's not clear to me what other less-engineered approaches would work.

...unless we actually have control over the 3rd party typing module (as opposed to the one in the stdlib). Then we could add experimental extensions directly, although it still feels a little weird to be putting experimental extensions in typing.

The more I think about the less I like that this module is named "typing.py". Yes, it's in a module namespace, but many people will probably prefer to write "from mypy_extensions import typing" and then use e.g. "typing.TypedDict", but if they also write "import typing" they have a name clash. And even if they don't it might confuse human readers to see "typing.TypedDict" (e.g. when using an IDE that hides the imports). Better not to invite confusion and give this a different name.

Perhaps mypy_extensions.mypy_typing as an alternate name. Here's some potential usage patterns:

from mypy_extensions.mypy_typing import TypedDict
Point = TypedDict('Point', dict(x=int, y=int))

from mypy_extensions import mypy_typing
Point = mypy_typing.TypedDict('Point', dict(x=int, y=int))

JukkaL · 2016-10-11T11:27:48Z

Why not just from mypy_extensions import TypedDict? I wonder if this is what Guido suggested.

I don't think that mypy_extensions should depend on anything currently in setup.py or mypy/git.py. Let's just keep the codebases separate -- the little sharing we could have is not worth the extra complexity and loss of flexibility. The git checks aren't relevant here, as they are primarily for typeshed, and mypy_extensions doesn't need it.

So here's how this would look better to me:

Only add new files mypy_extensions/mypy_extensions.py and mypy_extensions/setup.py (+ perhaps a readme and tests).
setup.py can be minimal. Here's an example for typing that is comparable, as it only installs a single file: https://github.com/python/typing/blob/master/setup.py
If we need separate implementations for Python 2 and 3, maybe have mypy_extensions/2.7 and mypy_extensions/3 subdirectories. However, perhaps a single implementation is sufficient, at least for now.
Version mypy_extensions independently of mypy.

gvanrossum · 2016-10-11T15:40:20Z

Amen.

davidfstr · 2016-10-11T19:20:20Z

Sounds good. I'll see about another iteration some time this week.

davidfstr · 2016-10-14T01:57:15Z

I've pushed a significantly leaner version of this package for review.

I've left mypy_extensions.py empty for the time being. If the thrust of this PR is generally good I might opt to merge it with #2206 so that I can actually put something in mypy_extensions, namely TypedDict.

gvanrossum

Thanks! I think the classifiers could be simpler, and can you please add a comment explaining what this is to mypy_extensions.py? Would also be handy if it said explicitly it has to be compatible with Python 2 and 3. Once you've got those two I'm ready to merge!

gvanrossum · 2016-10-14T21:35:28Z

+    'Topic :: Software Development',
+]
+
+classifiers = _mypy_classifiers + [


I see no reason not to make this into a single longer list.

davidfstr · 2016-10-15T01:06:52Z

Feedback integrated.

gvanrossum · 2016-10-15T01:47:03Z

Thanks for your patience! I really like the simplicity of the result.

davidfstr · 2016-10-15T02:33:56Z

Sure. These things take time.

davidfstr commented Oct 7, 2016

View reviewed changes

gvanrossum requested changes Oct 10, 2016

View reviewed changes

davidfstr force-pushed the ext branch from 0ffd193 to 2b02744 Compare October 14, 2016 01:51

Create mypy_extensions package.

a8cf5ab

davidfstr force-pushed the ext branch from 2b02744 to a8cf5ab Compare October 14, 2016 01:53

gvanrossum reviewed Oct 14, 2016

View reviewed changes

Merge classifiers. Improve docs.

518a70f

gvanrossum merged commit 47b1d6a into python:master Oct 15, 2016

		@@ -1,18 +1,28 @@
		"""Utilities for verifying git integrity."""

		from __future__ import print_function

		@@ -0,0 +1,18 @@
		from typing import cast, Dict, Type, TypeVar

Uh oh!

Uh oh!

Conversation

davidfstr commented Oct 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfstr commented Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JukkaL commented Oct 11, 2016

Uh oh!

gvanrossum commented Oct 11, 2016 via email

Uh oh!

davidfstr commented Oct 11, 2016

Uh oh!

davidfstr commented Oct 14, 2016

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfstr commented Oct 15, 2016

Uh oh!

gvanrossum commented Oct 15, 2016

Uh oh!

davidfstr commented Oct 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gvanrossum Oct 10, 2016 •

edited

Loading

gvanrossum Oct 10, 2016 •

edited

Loading

gvanrossum Oct 10, 2016 •

edited

Loading

davidfstr commented Oct 11, 2016 •

edited

Loading