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

Mypy 0.620 and generic type aliases #272

Merged
merged 3 commits into from Jul 31, 2018
Merged

Conversation

tailhook
Copy link
Contributor

@tailhook tailhook commented Jul 27, 2018

  • Fixes new check appeared in mypy 0.620
  • Also fixes usage of generic type aliases in some places (see below)

Things I've noticed:

  • MultiDictProxy.copy and CIMultiDictProxy.copy had wrong return type (fixed)
  • MultiDictProxy is inherited from a different class in .pyx and .py implementation
  • Coundn't enable disallow_any_generics = True in mypy config because there are a lot of errors like No library stub file for module 'pytest' (see travis). Not sure what to do with them.
  • Update: Using _S as a type name in MultiDict[_S, _T] confuses _S with a generic argument (TypeVar), while it's just an alias for Union. Probably better name it _Str or alike.

If disallow_any_generics is enabled, we get the following errors in
projects using multidict:

    multidict/__init__.pyi:34: error: Missing type parameters for generic type
    multidict/__init__.pyi:42: error: Missing type parameters for generic type
    multidict/__init__.pyi:58: error: Missing type parameters for generic type
Here are the original errors:

    multidict/__init__.pyi:16: error: Class multidict.MultiMapping has abstract attributes "__getitem__", "__iter__", "__len__"
    multidict/__init__.pyi:16: note: If it is meant to be abstract, add 'abc.ABCMeta' as an explicit metaclass
    multidict/__init__.pyi:28: error: Class multidict.MutableMultiMapping has abstract attributes "__delitem__", "__getitem__", "__iter__", "__len__", "__setitem__"
    multidict/__init__.pyi:28: note: If it is meant to be abstract, add 'abc.ABCMeta' as an explicit metaclass

Another things fixed:

* MultiDictProxy.copy returns MultiDict (not Proxy)
* CIMultiDictProxy.copy returns CIMultiDict (not Proxy)
@tailhook tailhook requested a review from asvetlov as a code owner July 27, 2018 12:48
@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #272 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #272   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files           4        4           
  Lines         355      355           
=======================================
  Hits          354      354           
  Misses          1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13d592e...c6d1d61. Read the comment docs.

@asvetlov asvetlov merged commit f0f1e1f into aio-libs:master Jul 31, 2018
@asvetlov
Copy link
Member

Thanks!

@tailhook
Copy link
Contributor Author

Any plans for a release?

@webknjaz
Copy link
Member

webknjaz commented Jul 31, 2018

We still need to test the flow with new tox-based approach, but it should be possible.

@asvetlov should I implement publishing to testpypi? For example with matching tag names (test-/-test/.dev0 prefix/suffix thing)? I did smth similar @ python/core-workflow#222 (python/core-workflow#262)

@tailhook
Copy link
Contributor Author

tailhook commented Aug 1, 2018

Yes, at least beta release would be helpful (can't install from git)

@webknjaz
Copy link
Member

webknjaz commented Aug 1, 2018

Why can't you?

@tailhook tailhook deleted the mypy620 branch August 1, 2018 12:35
@tailhook
Copy link
Contributor Author

tailhook commented Aug 1, 2018

Why can't you?

Can't find some *.c file as far as I remember. And there were no errors about cython, so I'm assuming it doesn't run cython on installation. Even if I'm missing something, pypi release would be more convenient anyway :)

@webknjaz
Copy link
Member

webknjaz commented Aug 1, 2018

it does run cython to generate C-files and then compiles shared objects out of that if you use sdist.
if you don't have build deps it will skip speed-ups and fall-back to pure-python implementation.
sdist published to pypi indeed contains C-files, so it's one dependency less (you don't have to need cython, just gcc). and wheels of course contain everything pre-compiled per platform.

We just didn't formalize any policy for publishing new versions, that's why I don't do it and it's fully on @asvetlov to trigger this. I'm only in charge of automation here.

@tailhook
Copy link
Contributor Author

tailhook commented Aug 1, 2018

it does run cython to generate C-files and then compiles shared objects out of that if you use sdist.

I'm doing pip install git+https://github.com/...

@asvetlov
Copy link
Member

asvetlov commented Aug 1, 2018

I've started to build the next 4.4.0 alpha version.
Let me check everything on alpha/beta and then make a release.

@webknjaz
Copy link
Member

webknjaz commented Aug 1, 2018

@tailhook do you have cython + gcc available in env?

@tailhook
Copy link
Contributor Author

tailhook commented Aug 1, 2018

gcc -- yes, cython -- no. But there were no errors like cython not found, so I've assumed that even if I install one it will not work anyway.

@webknjaz
Copy link
Member

webknjaz commented Aug 1, 2018

Yeah, it's being suppressed. So you get pure-python implementation available anyway.

@tailhook
Copy link
Contributor Author

tailhook commented Aug 1, 2018

I'm not sure this discussion is useful here, but even if cython errors are suppressed, gcc errors aren't. So I don't get package installed.

@webknjaz
Copy link
Member

webknjaz commented Aug 1, 2018

Actually, all of them should be suppressed. So it's likely a bug.

@tailhook
Copy link
Contributor Author

tailhook commented Aug 1, 2018

Well, if it's a bug, it's a good one. I don't want my programs to become slower without noticing. I'm not sure what is a common way, but I would prefer ALLOW_SLOW_IMPL=true pip install or somesuch, instead of silently installing pure python version.

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

@tailhook it's a common approach for all aio-libs repos. @asvetlov's motivation was to simplify stuff for non-techies.

I would prefer a warning to stderr instead of changing default behavior + there's aio-libs/aiohttp#3164 where we think of implementing an env var for unsuppressing errors in dev mode.

@tailhook
Copy link
Contributor Author

tailhook commented Aug 2, 2018

Here is the error BTW, and it's the same for 4.4.0a1, so it's not specific for git version:

    running build_ext
    building 'multidict._multidict' extension
    creating build/temp.linux-x86_64-3.7
    creating build/temp.linux-x86_64-3.7/multidict
    x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -g -fdebug-prefix-map=/build/python3.7-gdrLxR/python3.7-3.7.0=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/usr/include/python3.7m -c multidict/_multidict.c -o build/temp.linux-x86_64-3.7/multidict/_multidict.o -O2 -std=c99 -Wall -Wsign-compare -Wconversion -fno-strict-aliasing
    x86_64-linux-gnu-gcc: error: multidict/_multidict.c: No such file or directory
    x86_64-linux-gnu-gcc: fatal error: no input files
    compilation terminated.
    error: command 'x86_64-linux-gnu-gcc' failed with exit status 1

I would prefer a warning to stderr instead of changing default behavior

When building a container, you have almost no chance to see the warning. So warnings on installation are almost useless in my opinion.

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

Meaning warning in default mode and true error in strict mode.

Also, why do you use sdist, whereas you're supposed to use binary wheel?

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

Oh.. wheel haven't been published for some reason.

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

I've got a feeling that something's changed in setuptools or so.

cc @asvetlov

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

Apparently, travis config might be broken. @asvetlov it's on me.

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

I've triggered the release of v4.4.0a2: https://travis-ci.com/aio-libs/multidict/builds/80737712
This should fix things, at least linux-related. Still need to check what's going on with OS X jobs.

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

v4.4.0a3 should address OS X specific stuff: https://travis-ci.com/aio-libs/multidict/builds/80738972

@asvetlov
Copy link
Member

asvetlov commented Aug 2, 2018

@webknjaz thanks.
I'll try to find a way to deal with compiler errors the best.
Please give me a time (a couple days most likely) for experiments.

@webknjaz
Copy link
Member

webknjaz commented Aug 2, 2018

No worries, I feel like smth is missing there in env. I'll try to get it fixed in the evening.

@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2018

@tailhook
Copy link
Contributor Author

tailhook commented Sep 4, 2018

Great! Thanks! (now we need to fix aiohttp :) )

@webknjaz
Copy link
Member

webknjaz commented Sep 4, 2018

What's wrong with it? Anyway, file a ticket there. This is closed PR and comments are going to get lost

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