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

Prevent typing install in Python 3.6+ #126

Merged
merged 4 commits into from
Jan 9, 2021
Merged

Prevent typing install in Python 3.6+ #126

merged 4 commits into from
Jan 9, 2021

Conversation

damienallen
Copy link
Contributor

@damienallen damienallen commented Dec 28, 2020

Checklist

  • I read Contribution Guidelines
  • I ran the checks and the tests (make check && make test) before submitting the PR on my branch and they passed
  • I attached unit tests testing the provided code so the code coverage will not drop below 98%

Description

This change prevents pip from installing the typing backport package for python versions 3.6+ where the module is included in the standard library.

This is the officially recommended approach:
https://pypi.org/project/typing/

For package maintainers, it is preferred to use typing;python_version<"3.5" if your package requires it to support earlier Python versions. This will avoid shadowing the stdlib typing module when your package is installed via pip install -t . on Python 3.5 or later.

Why the change is needed?

In my case, docker builds are failing but the issue seems more widespread: python/typing#573

@apragacz
Copy link
Owner

@damienallen Hi, thanks very much for your contribution.

Any clue why this happens?
https://travis-ci.com/github/apragacz/django-rest-registration/jobs/465586611

rest_registration/checks.py:117: error: Argument 1 to "issubclass" has incompatible type "str"; expected "type"

@damienallen
Copy link
Contributor Author

Sorry, went fast and missed that. I will try it investigate and update the PR within the next week!

@damienallen
Copy link
Contributor Author

Can't pin down why this is happening. I also can't reproduce this locally with Python 3.6.10 and mypy 0.770 (using make install_dev and make mypy).

It seems that there is an issue with retrieving the attrs from rest_framework.settings.APISettings. If you look at the __getattr__() method:

    def __getattr__(self, attr):
        if attr not in self.defaults:
            raise AttributeError("Invalid API setting: '%s'" % attr)

        try:
            # Check if present in user settings
            val = self.user_settings[attr]
        except KeyError:
            # Fall back to defaults
            val = self.defaults[attr]

        # Coerce import strings into classes
        if attr in self.import_strings:
            val = perform_import(val, attr)

        # Cache the result
        self._cached_attrs.add(attr)
        setattr(self, attr, val)
        return val

This is where the logic lives to transform the module string from the settings dictionary into a proper import. This seems to work for me locally, so the mypy check passes. However, in travis, it seems that accessing the attr still returns a string which fails the issubclass check.

I'm not so familiar with unit-testing with django settings, does any of this ring any bells? Thoughts on how to repro this locally?

@apragacz
Copy link
Owner

apragacz commented Jan 9, 2021

@damienallen thanks for your investigation.
I was able to reproduce the issue with a fresh virtualenv and installing dependencies from scratch. In my case it was based on Python 3.6.11 but I don't think that's the issue here.
I noticed that some packages may have incompatible versions (mypy, django-stubs,djangorestframework-stubs), will try to straighten that out.

@apragacz
Copy link
Owner

apragacz commented Jan 9, 2021

@damienallen looks there was some regression introduced with version djangorestframework-stubs==1.3.0, so I downgraded it for now.

Unfortunately I don't have time now to do deeper investigation now to drop some reasonable github issue for that lib :(

@apragacz apragacz merged commit 364a21d into apragacz:master Jan 9, 2021
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.

2 participants