attrs icon indicating copy to clipboard operation
attrs copied to clipboard

Ship attrs mypy plugin ourselves

Open hynek opened this issue 7 years ago • 15 comments

I don’t know how far this is aspirational, but https://github.com/python/mypy/issues/3916 gives me some hope?

hynek avatar Aug 06 '18 04:08 hynek

ehm ping @chadrik @euresti @ethanhs

hynek avatar Aug 08 '18 06:08 hynek

I don't think there is anything that should block this. It might take some careful extraction, and it will require people to point to attr.mypy or some such location in their mypy.ini (and use the latest mypy), but otherwise I think everything should work.

emmatyping avatar Aug 08 '18 07:08 emmatyping

Yep, my PR for externally configured mypy plugins got merged! Thanks @ethanhs for the quick review.

Here are a few questions and complicating factors, off the top of my head:

  • How does a user install the plugin? the plugin is only supported on python3, and the interpreter used for type checking is not necessarily the same as the version used at runtime. Should the plugin be a submodule (attr.mypy) or a separate module (attr_mypy)? Note, I don't think that attrs is a dependency of the plugin (@euresti can you confirm, pls). Instinct tells me the plugin should be a completely separate module on PyPI (python3 -m pip install attrs_mypy), though the code can still live here for simplicity.
  • I think we'll need to start running the tests for the plugin (bc if we don't then who will?), but the mypy test runner is not supported as a public API. I originally tried to make a PR against mypy to make it easier to use their test runner in an external project and it was roundly rejected
  • how many versions of mypy should we test and support? only the latest?

chadrik avatar Aug 08 '18 18:08 chadrik

How does a user install the plugin?

If we do go the separate package route, it should probably be mypy-attrs, which follows the style of pytest-x for pytest plugins (and is a common pattern among PyPi packages for plugins in general).

Note, I don't think that attrs is a dependency of the plugin

It is not, mypy does not depend on attrs (except for tests around this).

I think we'll need to start running the tests for the plugin (bc if we don't then who will?), but the mypy test runner is not supported as a public API.

I'm still working on it, but I am getting happier with the pytest plugin I am writing for data driven test cases. I'm hoping to spend some time on it this week.

emmatyping avatar Aug 08 '18 19:08 emmatyping

discussion moved to #480

hynek avatar Feb 02 '19 13:02 hynek

I'm reopening this issue so we can discuss moving the plugin into attrs.

Previously I said:

Being absolutely honest I prefer to leave the plugin as part of mypy for the following reasons:

  • If someone modifies something that breaks the plugin tests they'll have to go and figure out how to fix it.
  • If someone submits a PR there will be mypy devs looking at it who have way more knowledge than we do about how mypy works.
  • The one obvious annoyance is the version skew. I can't decide if it's a necessary evil or if it's the straw that breaks the camel's back. I will note that the version skew could happen in either situation. Maybe we move the plugin to attrs and then a new version of mypy breaks our plugin in a past version of attrs.

However my PR https://github.com/python/mypy/pull/9396 has been sitting there unreviewed for 2 months now. Note: They aren't ignoring me. They are just overwhelmed with work and not enough people.

So I think it might be best if we don't depend on their PRs. Note it would make mypy and attrs no longer plug and play. You'd have to install mypy and attrs and then edit your mypy.ini to tell it about the plugin.

Another option is to ship the plugin and stubs in a separate package. mypy-attrs for example.

euresti avatar Nov 10 '20 14:11 euresti

Yeah I'm sure they got hit quite hard by Dropbox switching incrementally away from Python. And I mean we have access to all relevant plugin devs here too…

What would be the upside of an own package? I really feel like CalVer and potentially frequent releases go great with integrating the plugin?

hynek avatar Nov 10 '20 14:11 hynek

I'm not sure why other projects go with using a separate package. Maybe it's to not have code that a user won't actually use. But I'm ok if it's all bundled together.

euresti avatar Nov 10 '20 15:11 euresti

I also think that would make stuff just more brittle and confusing. This way we get “here’s attrs 20.x, and that’s all there is to it”.

hynek avatar Nov 10 '20 15:11 hynek

What about packages that build on attrs but add their own wrappers for attr.s, attr.dataclass or attr.ib. Would they need to ship their own plugin for the attrs-mypy-plugin? Or could we just add the corresponding functions to the plugin shipped with attrs?

sscherfke avatar Nov 10 '20 19:11 sscherfke

I use that myself all the time so failing that functionality would be a no-go. :)

hynek avatar Nov 11 '20 07:11 hynek

Doing a bit of research of how to bring the tests over. It looks like sqlalchemy-stubs does this: https://github.com/dropbox/sqlalchemy-stubs/blob/master/test/testsql.py

I'll see if porting that over to our system is easy.

Also any problem with the licenses in bringing the plugin code from mypy to here? I can't imagine, but I'm pretty noob on open source development.

euresti avatar Dec 13 '20 16:12 euresti

Didn't you literally write it?

hynek avatar Dec 13 '20 16:12 hynek

(Once you open the PR on the attrs repo, we can ask the other contributors for permission. mypy is MIT so it would be probably fine to add a header but I'd like to prevent licensing inconsistencies)

hynek avatar Dec 13 '20 16:12 hynek

I did write most of it, but others have contributed fixes and patches.

euresti avatar Dec 13 '20 17:12 euresti