Skip to content
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

Add ordered-set-stubs installation instructions #42

Merged
merged 1 commit into from Oct 10, 2018
Merged

Add ordered-set-stubs installation instructions #42

merged 1 commit into from Oct 10, 2018

Conversation

rominf
Copy link
Contributor

@rominf rominf commented Sep 16, 2018

I have found that:

  1. ordered_set.pyi file is not copied to ordered_set package.
  2. Add support for typing annotations #26 is not fixed, OrderedSet[str], for example, gives an error.

This PR fixes both.

After the merge, please release. Sorry, I didn't notice for the first time.

Copy link
Owner

@rspeer rspeer left a comment

Choose a reason for hiding this comment

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

I don't understand how this typing ecosystem is supposed to work anymore. And of course I can't test whether it does the right thing in your IDE or your CI or whatever needs these types, and that seems to be the issue here.

If this is something I should really be doing to such a simple library as ordered-set, then there would have to be some well-established guide I can follow about how to test and integrate .pyi files. But the integration pattern where I release new versions to PyPI and then you tell me if they worked isn't sustainable.

If you're in a hurry to cope with some policy that requires type checks to pass, I suggest forking ordered-set.

setup.py Outdated
@@ -15,8 +16,11 @@
long_description_content_type='text/markdown',
py_modules=['ordered_set'],
package_data={'': ['MIT-LICENSE']},
data_files=(('lib/python{}.{}/site-packages'.format(*sys.version_info[:2]), ['ordered_set.pyi']),
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't look right to me.

I tried searching for things related to this. I found python/typing#84 (comment), where lines like this appear to be a hack that MyPy developers were using to experiment with installable types.

The "ideal UX for library authors" suggests that this should be unnecessary, that .pyi files should just be pip installed the same way as .py files. If this doesn't work yet, then I think that is something for pip and mypy to work out between them, instead of every package maintainer adding fragile-looking path acrobatics to their library. On the other hand, if this really is just boilerplate that the Python devs promise will keep working, just point me to the guide.

ordered_set.py Outdated
T = TypeVar('T')


class OrderedSet(MutableSet[T], Sequence[T]):
Copy link
Owner

Choose a reason for hiding this comment

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

Is there no way to specify that an OrderedSet[T] is a Sequence[T] in the .pyi file? We have to import typing in the code itself?

If so, what is the point of .pyi files? I thought the entire goal was that .pyi files could be written separately to describe a library that already exists.

Copy link
Contributor Author

@rominf rominf Sep 20, 2018

Choose a reason for hiding this comment

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

Is there no way to specify that an OrderedSet[T] is a Sequence[T] in the .pyi file? We have to import typing in the code itself?

I specified this in the .pyi file, but that's not enough: the .pyi file is not executed, but the .py file is, and when you run OrderedSet[int], for example, python looks at the OrderedSet definition in the .py file. Moreover, I don't see anything bad in using the typing library. It's just a tiny dependency, which you will drop when the time comes and old python will be not longer supported.

If so, what is the point of .pyi files? I thought the entire goal was that .pyi files could be written separately to describe a library that already exists.

The point of .pyi files is that you can describe types with convenient type annotations convention even if you use old python. I don't see the point to annotate existing library, that supports only python > 3.5, that way.

@rspeer
Copy link
Owner

rspeer commented Sep 17, 2018

I found the PEP that discusses typing stubs. It does not recommend the path tricks that appear in this PR. Instead, it recommends adding a marker file named py.typed to the package data, and then setuptools is supposed to do the right thing. https://www.python.org/dev/peps/pep-0561/#packaging-type-information

It also seems possible that you could create an ordered-set-stubs package, as described in "Stub-only packages", if you can untangle the thing that required OrderedSet to explicitly inherit from type-annotated things in the code.

@rominf
Copy link
Contributor Author

rominf commented Sep 20, 2018

I found the PEP that discusses typing stubs. It does not recommend the path tricks that appear in this PR. Instead, it recommends adding a marker file named py.typed to the package data, and then setuptools is supposed to do the right thing. https://www.python.org/dev/peps/pep-0561/#packaging-type-information

Thank you. I didn't notice myself. Fixed it, see the last commit.

It also seems possible that you could create an ordered-set-stubs package, as described in "Stub-only packages", if you can untangle the thing that required OrderedSet to explicitly inherit from type-annotated things in the code.

  1. I don't think it's possible to not inherit from type-annotated classes.
  2. I don't see the point to create the separate package. Why create the separate package if we can include types in the main package?

@rspeer
Copy link
Owner

rspeer commented Sep 20, 2018

Importing typing inside the code adds an install-time dependency and a run-time cost to the code. ordered_set has gotten by as a single module with no dependencies for 6 years. Someone could decide they want nothing to do with setuptools and just drop ordered_set on their PYTHONPATH, and it would still work. I'd hate to lose that over types.

I'm fine with having the types in the package, as long as they can go in the .pyi file.

The main reason I bring up the ability to make a separate package is as an existence proof: clearly you can add types in .pyi files without changing the .py code, because people add types for code they don't control at all.

@rominf
Copy link
Contributor Author

rominf commented Sep 29, 2018

@rspeer, I asked the question on stackoverflow. Let's see if somebody has the solution for this.

@rominf
Copy link
Contributor Author

rominf commented Sep 29, 2018

OK, I updated PR, I don't inherit from typing classes now. Also updated #26.

@rspeer, can you accept PR now?

@rspeer
Copy link
Owner

rspeer commented Sep 29, 2018

Well, that's simple enough. Except, don't you need to actually add an empty file named py.typed for the installation of types to work?

@rominf
Copy link
Contributor Author

rominf commented Sep 30, 2018

Except, don't you need to actually add an empty file named py.typed for the installation of types to work?

I think you are right. My bad.

The problem is that despite I did it, it didn't work out: ordered_set.pyi didn't get included. I asked a question on stackoverflow.com.

@rominf rominf changed the title Add ordered_set.pyi to setup.py, inherit from classes from typing Add ordered_set.pyi to setup.py Oct 1, 2018
@rspeer
Copy link
Owner

rspeer commented Oct 1, 2018

It seems likely to me that distutils doesn't know anything about py.typed and that it requires a current version of pip. But that would break the package distribution process, because pip doesn't replace the sdist command, as far as I know.

If SO doesn't get you a reply, perhaps raise an issue on the MyPy issue tracker.

From the state of MyPy discussion, it seems likely to me that support for packaging types is still immature, despite the PEP. They might actually do this only through the typeshed project. But I could be wrong -- maybe they got something into Python 3.7 that checks for py.typed and installs types.

@rspeer
Copy link
Owner

rspeer commented Oct 1, 2018

I've tried a couple of variations on this in my Python 3.7 environment that I'm certain is up to date with setuptools and stuff.

I can get the .pyi file included in the sdist package if I put it in MANIFEST.in, but it still doesn't seem to get installed anywhere. (I don't know where to look, though. Where even is shared/typehints?)

@rspeer
Copy link
Owner

rspeer commented Oct 1, 2018

Oh, I figured it out. py.typed is something that's supposed to go in the package, and technically, ordered_set doesn't have any packages. It has a module.

In PEP 561, I found: "This PEP does not support distributing typing information as part of module-only distributions. The code should be refactored into a package-based distribution and indicate that the package supports typing as described above."

I also found the trick you were doing before with the explicit shared/typehints path in PEP 484. Did that work? It was ugly, but I'd rather go with that (now that I know it's in a PEP) than make such a big change to the packaging of ordered_set.

@ethanhs
Copy link

ethanhs commented Oct 1, 2018

Hi, I would not recommend relying on shared/typehints it is not implemented in mypy or any other type checker to my knowledge, and PEP 561 is supposed to replace it.

Instead, I'd recommend that you create an ordered_set-stubs package and add that to your setup.py. (You may want to read https://mypy.readthedocs.io/en/latest/installed_packages.html#making-pep-561-compatible-packages)

@rominf
Copy link
Contributor Author

rominf commented Oct 2, 2018

@rspeer, should I create ordered_set-stubs package as @ethanhs recommended?

@rspeer
Copy link
Owner

rspeer commented Oct 2, 2018

I suppose so. Maybe the stubs can be a real package even when ordered_set isn't, and that'll make it work.

Thank you again for all the effort you're putting into this, and I wish I could determine more reliably what the right way to do things is.

@rominf
Copy link
Contributor Author

rominf commented Oct 2, 2018

@rspeer, OK. I suppose I will do it at weekend.

@rominf rominf changed the title Add ordered_set.pyi to setup.py Add ordered-set-stubs installation instructions Oct 7, 2018
@rominf
Copy link
Contributor Author

rominf commented Oct 7, 2018

@rspeer, I've created ordered-set-stubs, published it on github.com and pypi.org. Here I'm adding information on how to use type hinting by installing this package.
I've tested that everything works as expected on the test project: https://github.com/rominf/ordered-set-stubs-test

@rspeer rspeer merged commit 919e958 into rspeer:master Oct 10, 2018
@rspeer
Copy link
Owner

rspeer commented Oct 10, 2018

Excellent, thanks!

@rominf
Copy link
Contributor Author

rominf commented Oct 10, 2018 via email

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.

None yet

3 participants