Skip to content

Conversation

@ambv
Copy link
Contributor

@ambv ambv commented Apr 6, 2018

This enables us to update the typeshed package on PyPI which is currently
woefully outdated.

Lukasz Langa added 2 commits April 6, 2018 11:57
This enables us to update the `typeshed` package on PyPI which is currently
woefully outdated.
@JelleZijlstra
Copy link
Member

I didn't even know there was a pypi package. What is it used for?

@ambv
Copy link
Contributor Author

ambv commented Apr 6, 2018

I don't know if anybody is using it. I tried to use it in a few settings in the past but it never seems to be up-to-date enough.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

This feels over-engineered... Did you copy a bunch of boilerplate from elsewhere? Why not start with the smallest possible setup.py file that could possibly work?

Also, how do you expect mypy would find this? There probably needs to be some design for how that is supposed to work before we land anything here.

setup.py Outdated
@@ -0,0 +1,56 @@
import ast
Copy link
Member

Choose a reason for hiding this comment

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

Why are you importing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's from a previous version, will remove.

__all__ = ['__location__', '__version__']


def is_git_repo(dir):
Copy link
Member

Choose a reason for hiding this comment

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

Why is all this git stuff 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.

Mypy has a very similar thing. There are a few reasons to want this:

  • we want a "dirty" check for building a wheel for PyPI
  • we want to know the exact commit that a release was built from and publish it on PyPI, too (I'm doing that via project_urls)

Copy link
Member

Choose a reason for hiding this comment

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

I still don't think it should be in the "typeshed" package. It's a private API used only by setup.py. So I think all this code might as well move into setup.py (the amount of code stays constant and the isolation improves).

(That still feels wrong: setup.py is not just the thing you use to build a wheel -- it's the thing you use to install from any other source as well. If you want to have additional assurances that you're not uploading a wheel built from a dirty working directory, ideally you should have a separate helper script. But I don't have an answer for how to make the source_url specific to a commit in that case, so I'll tolerate it in setup.py. But I still don't think it belongs in typeshed/__init__.py.)

setup.py Outdated
return ld_file.read()


assert not typeshed.__version__.endswith(".dirty"), (
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mypy and projects I manage have similar checks, we don't want random dirty changes to land on PyPI without us knowing, right?

Copy link
Contributor Author

@ambv ambv left a comment

Choose a reason for hiding this comment

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

@gvanrossum, every setup.py file is an amalgamation of things we've done previously but this one is pretty minimal. The setup call is 22 lines long out of which over 1/3 is Trove classifiers.

What would a "minimal" setup.py achieve vs. what is here?

setup.py Outdated
return ld_file.read()


assert not typeshed.__version__.endswith(".dirty"), (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mypy and projects I manage have similar checks, we don't want random dirty changes to land on PyPI without us knowing, right?

setup.py Outdated
@@ -0,0 +1,56 @@
import ast
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's from a previous version, will remove.

__all__ = ['__location__', '__version__']


def is_git_repo(dir):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mypy has a very similar thing. There are a few reasons to want this:

  • we want a "dirty" check for building a wheel for PyPI
  • we want to know the exact commit that a release was built from and publish it on PyPI, too (I'm doing that via project_urls)

@gvanrossum
Copy link
Member

gvanrossum commented Apr 13, 2018 via email

@ambv
Copy link
Contributor Author

ambv commented Apr 13, 2018

OK. I simplified it a bit. Now it doesn't fail an assert on dirty changes.

The file is now 50 lines long vs. mypy_extensions' 44 lines. In the additional six lines, it offers the following functionality:

  • __version__ stored in typeshed/__init__.py enabled users to query it at runtime
  • Markdown-based long description is read straight from README.md
  • Git commit information is stored in PyPI metadata (via the "Source Code" link)
  • Warns the caller on stderr if there are uncommitted changes

I hope this is a good compromise now. Further simplification will sacrifice the functionality above and I think those things are pretty essential.

@ambv
Copy link
Contributor Author

ambv commented Apr 13, 2018

I have a more interesting thing to bikeshed on.

Versioning scheme

@gvanrossum, you mentioned you'd prefer using versions from mypy. Instead, I'd like to propose a mypy-independent versioning scheme called CalVer. Variants of it are used by Ubuntu, Twisted, attrs, pytz, PyOpenSSL, black, bugbear, and many others.

The variant I'd like to suggest is:

version = f"{YY}.{MM}.{n}"

YY = 18  # two digit year
MM = 4   # single or two digit month
n = 0    # incremented integers per release, reset every month

Semantic versioning of typeshed doesn't make sense because the project has a lot of moving parts and there's no clear distinction between "features" and "fixes". There's also no clear goal to reach for a "stable" version.

@gvanrossum
Copy link
Member

Given the discussion we had off-line on Wednesday maybe we should discuss this in person at PyCon? IIRC you seemed to like some kind of synchronization between mypy and typeshed releases, and the only way I can see that happening is if the version numbers match.

matrix:
include:
- python: "3.6-dev"
env: TEST_CMD="flake8"
Copy link
Member

Choose a reason for hiding this comment

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

I think I saw a separate PR for this recently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of. Jelle's PR was about upgrading the version of flake8. But I'll move this to the config where it belongs better.

include *.rst *.md LICENSE
recursive-include typeshed *.py
recursive-include typeshed/stdlib *.pyi
recursive-include typeshed/third_party *.pyi
Copy link
Member

Choose a reason for hiding this comment

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

Can't the latter two be combined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those directories are symlinks and I couldn't get it to work with just one line. Globbing inside those directories works fine though.

Copy link
Member

Choose a reason for hiding this comment

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

OK

name="typeshed",
version=PUBLIC_VERSION,
description="Collection of library stubs for Python, with static types",
long_description=get_long_description(),
Copy link
Member

Choose a reason for hiding this comment

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

Where is the long description shown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the document on https://pypi.org/project/typeshed/

setup.py Outdated
import typeshed


if typeshed.__version__.endswith(".dirty"):
Copy link
Member

Choose a reason for hiding this comment

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

I still don't like that there is a typeshed package whose __init__.py runs a bunch of subprocesses, just so that setup.py can use a dynamically generated __version__. (For mypy I don't mind since it's an app not a library.) Other projects I've seen just put the version number in setup.py -- if you're going to dynamically generate the version why not put the whole thing in setup.py? And why does typeshed need to be a package? Who besides setup.py would ever import it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I agree that subprocessing with every import of typeshed is spurious. I'll change that.

…ve flake8 exclusions to config file, annotate functions in __init__.py, move `is_dirty()` check to after setup.py so it's visible, make setup.py work on Python 2.7
@ambv
Copy link
Contributor Author

ambv commented Apr 16, 2018

OK, made changes you wanted.

Re: version

(PyCon is still 3 weeks away, we'd like an updated typeshed release by that time on PyPI. So talking about the versioning scheme in person is a bit late.)

I don't follow how tying release schedules of mypy and typeshed requires matching version numbers. Especially that as you said, the mypy project isn't interested in switching to using PyPI releases of typeshed in the foreseeable future.

To tie release schedules, all that's required is for mypy to release a candidate of the next typeshed on PyPI at the start of the cool-off period before each release and to pin the submodule to the typeshed commit that tagged the release candidate.

For example, in the cool-off period for mypy 0.600 the best scenario would be simply:

  • we tag and release typeshed 18.5.0rc1 on PyPI.
  • we pin the typeshed submodule in mypy to the commit that tagged 18.5.0rc1.
  • we call for other projects to test it.
  • Everything is fine!
  • we tag and release typeshed 18.5.0 on PyPI. (same contents but gold version)
  • we pin the typeshed submodule in mypy to the commit that tagged 18.5.0.
  • we release mypy 0.600

And the worst case scenario would look like this:

  • we tag and release typeshed 18.5.0rc1 on PyPI.
  • we pin the typeshed submodule in mypy to the commit that tagged 18.5.0rc1.
  • we call for other projects to test it.
  • Oops, we discover a problem in typeshed!
  • we fix the problem
  • we tag and release typeshed 18.5.0rc2 on PyPI.
  • we pin the typeshed submodule in mypy to the commit that tagged 18.5.0rc2.
  • we call for other projects to test it.
  • Oops, this commit has a problem with PyCharm or other tools!
  • we fix the problem
  • we tag and release typeshed 18.5.0rc3 on PyPI.
  • we pin the typeshed submodule in mypy to the commit that tagged 18.5.0rc3.
  • we call for other projects to test it.
  • Everything is fine!
  • we tag and release typeshed 18.5.0 on PyPI. (same contents but gold version)
  • we pin the typeshed submodule in mypy to the commit that tagged 18.5.0.
  • we release mypy 0.600

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

We also still need to have a discussion about the versioning scheme. I'm actually fine with using CalVer but I don't want to make any promises about what we do in mypy -- I haven't seen a good argument why we should change what mypy currently does. If that's fine with you then we can go with CalVer here.

include *.rst *.md LICENSE
recursive-include typeshed *.py
recursive-include typeshed/stdlib *.pyi
recursive-include typeshed/third_party *.pyi
Copy link
Member

Choose a reason for hiding this comment

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

OK

__all__ = ['__location__', '__version__']


def is_git_repo(dir):
Copy link
Member

Choose a reason for hiding this comment

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

I still don't think it should be in the "typeshed" package. It's a private API used only by setup.py. So I think all this code might as well move into setup.py (the amount of code stays constant and the isolation improves).

(That still feels wrong: setup.py is not just the thing you use to build a wheel -- it's the thing you use to install from any other source as well. If you want to have additional assurances that you're not uploading a wheel built from a dirty working directory, ideally you should have a separate helper script. But I don't have an answer for how to make the source_url specific to a commit in that case, so I'll tolerate it in setup.py. But I still don't think it belongs in typeshed/__init__.py.)

@gvanrossum
Copy link
Member

We also still need to have a discussion about the versioning scheme.

Obviously I missed your separate message. :-) Will reply separately.

@gvanrossum
Copy link
Member

(PyCon is still 3 weeks away, we'd like an updated typeshed release by that time on PyPI. So talking about the versioning scheme in person is a bit late.)

I know I'm bound by an NDA, but for others who weren't at our meeting last week, the pressure to have this sorted strictly before PyCon may sound a bit strange. I'll leave it up to you to clarify why you're in a hurry (or leave it a mystery, if you prefer).

I don't like that you're proposing extra work for the mypy team without any added benefits for us. Here's my counter-proposal: We do the mypy release exactly like we did before and then once we're ready to release mypy we also release typeshed at the same commit as used by mypy.

AFAIK PyCharm's release cycle is much longer than mypy's so I don't think we should worry about reaching a fixed point where a given typeshed release works with all tools that use it. If PyCharm discovers they can't use typeshed 18.5.0 (final) they can submit a PR and they can help release typeshed 18.5.1 that includes it.

@ambv
Copy link
Contributor Author

ambv commented Apr 16, 2018

If PyCharm discovers they can't use typeshed 18.5.0 (final) they can submit a PR and they can help release typeshed 18.5.1 that includes it.

Fair enough.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Way better! Can you add documentation somewhere for how to do releases and also for how import typeshed should be used? These may be in the form of docstrings or comments in the source.



# Deprecated name kept for backwards compatibility.
typeshed = __location__
Copy link
Member

Choose a reason for hiding this comment

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

Is this just for compatibility with the antiquated typeshed package currently on PyPI? Isn't it better to just get rid of it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't want to break whatever usage is there in the wild but you're probably right that there might not be any.

except ImportError:
__location__ = os.path.dirname(os.path.dirname(__file__))
else:
__location__ = pkg_resources.resource_filename("typeshed", "")
Copy link
Member

Choose a reason for hiding this comment

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

What is the significance of __location__? I don't recall the docs and neither does Google.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No significance, just like __version__. It's just a name for people to use to figure out where their stubs live:

import typeshed

subprocess.run(["mypy", f"--custom-typeshed-dir={typeshed.__location__}"])

I didn't want to use __path__ because that one does change where importlib would look for sub-packages and that would break for the pkg_resources version which unpacks typeshed to a temporary directory.

Do you have a different suggestion for this name?

Copy link
Member

Choose a reason for hiding this comment

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

Ah. That's why comments are useful. :-)

I'd avoid using a dunder name, since that suggests a connection with official Python stuff. (Technically all dunder names are reserved by Python.)

I'd name this typeshed_location or typeshed_dir (the latter matching --custom-typeshed-dir more closely). You can even make it a function if you want to be fancy -- that makes it easier to turn it into something algorithmically complex.

Also, will the fancy resource_filename() call actually work if, say, typeshed was installed as a zip file? Otherwise is there a reason not to just use the fallback based on __file__ always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Technically all dunder names are reserved by Python.)

True but that ship has long sailed for metadata like __version__, __author__, etc. The standard library alone has 67 and 31 instances of those, respectively. I wanted __location__ to be consistent with __version__ but you're right that it's probably not worth it as it might be confusing.

Since we are already importing typeshed, including "typeshed" in the name of the variable feels icky to me. I'll just make a function called get_dir().


Also, will the fancy resource_filename() call actually work?

Yes, the pkg_resources docs state that:

If the resource is in an archive distribution (such as a zipped egg), it will be extracted to a cache directory, and the filename within the cache will be returned.

I tested this and it works. This is why I set zip_safe to True in setup.py. The good thing about making get_dir() a function is that unless called, the import alone won't extract typeshed to a temporary directory in this situation.

Copy link
Member

Choose a reason for hiding this comment

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

OK, get_dir() sounds fine. (And as you saw __version__ and __author__ have some kind of semi-official status, though I can't find a PEP -- probably the convention predates the PEP system.)

Happy that pkg_resources is thorough!

@ambv
Copy link
Contributor Author

ambv commented Apr 16, 2018

OK, will add docs.

import os
# -*- coding: utf-8 -*-
"""Typeshed is a collection of type annotations for the standard library and
third-party libraries.
Copy link
Member

Choose a reason for hiding this comment

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

This violates PEP 257 -- the first paragraph should fit on one line.

third-party libraries.
The type annotations in question are stored in so-called "stub files". They're
Python-like files with the *.pyi file extension. Those files are parseable by
Copy link
Member

Choose a reason for hiding this comment

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

Technically the * isn't part of the extension. ;-)

The type annotations in question are stored in so-called "stub files". They're
Python-like files with the *.pyi file extension. Those files are parseable by
the built-in Python AST but are not executable. Functions and methods in stub
Copy link
Member

Choose a reason for hiding this comment

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

But why would you parse them with the built-in ast module? That loses # type: ignore. Maybe it's better to just refer to PEP 484 for a discussion of what stubs are? Or the toplevel README.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will change.

└── third_party
├── 2
├── 2and3
└── 3
Copy link
Member

Choose a reason for hiding this comment

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

There can be 3.4 etc. here too (though I'm elated there aren't any currently :-).

Modules with types that behave the same regardless of the Python version used
are stored in `2and3` directories. Specific versions like `3.6` describe
modules which are not available in previous versions. `3` is a generic
directory storing type informartion for any Python 3 version. Those stub files
Copy link
Member

Choose a reason for hiding this comment

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

"Those" seems unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

directory storing type informartion for any Python 3 version. Those stub files
often contain sections which declare compatibility with only a particular subset
of Python 3 versions. Those sections are marked using `if sys.version_info`
checks.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe link this to the relevant section in PEP 484 too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

the built-in Python AST but are not executable. Functions and methods in stub
files don't contain bodies. Variables and attributes don't contain real values.
Typeshed is organized in the following directory structure:
Copy link
Member

Choose a reason for hiding this comment

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

Nice, but this has some overlap with the description in CONTRIBUTING.md. Maybe more should be in there? Or in README.md? I'm trying to avoid a situation where you have to piece the full details from too many different sources -- often when something changes some of the documentation sources isn't updated and causes confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. I'd like to keep this because README.md is not detailed enough and CONTRIBUTING.md isn't part of the distribution. I'd like help() to be descriptive enough on its own.

of Python 3 versions. Those sections are marked using `if sys.version_info`
checks.
To get the absolute location of the base typeshed directory in your project,
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is the key information that's needed in this file.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Almost there. I'd like to see some explicit instructions on how to do a release, perhaps as a docstring in setup.py (since when I wanted to do a release and didn't remember how to do it that's where I'd start reading).

@gvanrossum
Copy link
Member

So what happened? Did you stop caring or are you waiting for me? IIUC there's one task (add instructions for doing a release) and the tests are currently failing.

@ambv
Copy link
Contributor Author

ambv commented Apr 26, 2018

Tests failures were unrelated, should be gone when I rebase. Yes, I need to write the guide. I'll get something brief out and then we can iterate on the wording.

@JelleZijlstra
Copy link
Member

@ambv are you still interested in this?

@gvanrossum
Copy link
Member

@ambv I am closing this PR. If you're still interested you can reopen it.

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.

4 participants