Skip to content

Ship attrs mypy plugin ourselves #421

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

Open
hynek opened this issue Aug 6, 2018 · 15 comments
Open

Ship attrs mypy plugin ourselves #421

hynek opened this issue Aug 6, 2018 · 15 comments
Labels

Comments

@hynek
Copy link
Member

hynek commented Aug 6, 2018

I don’t know how far this is aspirational, but python/mypy#3916 gives me some hope?

@hynek
Copy link
Member Author

hynek commented Aug 8, 2018

ehm ping @chadrik @euresti @ethanhs

@emmatyping
Copy link
Member

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.

@chadrik
Copy link
Contributor

chadrik commented Aug 8, 2018

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?

@emmatyping
Copy link
Member

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.

@hynek
Copy link
Member Author

hynek commented Feb 2, 2019

discussion moved to #480

@hynek hynek closed this as completed Feb 2, 2019
@euresti
Copy link
Contributor

euresti commented Nov 10, 2020

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 python/mypy#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 euresti reopened this Nov 10, 2020
@hynek
Copy link
Member Author

hynek commented Nov 10, 2020

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?

@euresti
Copy link
Contributor

euresti commented Nov 10, 2020

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.

@hynek
Copy link
Member Author

hynek commented Nov 10, 2020

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”.

@sscherfke
Copy link
Contributor

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?

@hynek
Copy link
Member Author

hynek commented Nov 11, 2020

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

@euresti
Copy link
Contributor

euresti commented Dec 13, 2020

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.

@hynek
Copy link
Member Author

hynek commented Dec 13, 2020

Didn't you literally write it?

@hynek
Copy link
Member Author

hynek commented Dec 13, 2020

(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)

@euresti
Copy link
Contributor

euresti commented Dec 13, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants