swh/web/api/throttling.py
74	why remove `is not None`?
88–89	Could you add a test for `request.method` being None?
151	What about this? T = TypeVar('T', Callable) def decorator(func: T) -> T: And this might allow you to completely specify the type of `throttle_scope`.

This revision now requires changes to proceed.Mar 20 2020, 11:35 AM

anlambert added inline comments.Mar 20 2020, 2:10 PM

swh/web/api/throttling.py
74	I got mistaken by mypy, will revert.
88–89	This should never be the case (https://stackoverflow.com/questions/7836834/how-can-request-method-none-in-django) so no test needed. I was forced to do that as `django-stubs` types `method` attribute as `Optional[str]` because it is initialized to `None` in `HttpRequest` constructor. So I got the following mypy error: swh/web/api/throttling.py:83: error: Item "None" of "Optional[str]" has no attribute "lower" I will go for a cast instead: method = cast(str, request.method) request_scope = f'{default_scope}_{method.lower()}'
151	Thanks for the tip, got it working with: from rest_framework.views import APIView View = TypeVar('View', bound=APIView) def decorator(func: View) -> View:

vlorentz added inline comments.Mar 20 2020, 2:21 PM

swh/web/api/throttling.py
88–89	I'd rather use an assertion

anlambert added inline comments.Mar 20 2020, 2:21 PM

swh/web/api/throttling.py

151

mypy is okay but unfortunately webapp execution got broken due to recursive import:

Traceback (most recent call last):
  File "swh/web/manage.py", line 46, in <module>
    execute_from_command_line(sys.argv)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/base.py", line 361, in execute
    self.check()
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/base.py", line 390, in check
    include_deployment_checks=include_deployment_checks,
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/commands/migrate.py", line 65, in _run_checks
    issues.extend(super()._run_checks(**kwargs))
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/management/base.py", line 377, in _run_checks
    return checks.run_checks(**kwargs)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/checks/registry.py", line 72, in run_checks
    new_errors = check(app_configs=app_configs)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/checks/urls.py", line 13, in check_url_config
    return check_resolver(resolver)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/core/checks/urls.py", line 23, in check_resolver
    return check_method()
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/urls/resolvers.py", line 399, in check
    for pattern in self.url_patterns:
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/utils/functional.py", line 80, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/urls/resolvers.py", line 584, in url_patterns
    patterns = getattr(self.urlconf_module, "urlpatterns", self.urlconf_module)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/utils/functional.py", line 80, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/urls/resolvers.py", line 577, in urlconf_module
    return import_module(self.urlconf_name)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/antoine/swh/swh-environment/swh-web/swh/web/urls.py", line 38, in <module>
    url(r'^api/', include('swh.web.api.urls')),
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/django/urls/conf.py", line 34, in include
    urlconf_module = import_module(urlconf_module)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/antoine/swh/swh-environment/swh-web/swh/web/api/urls.py", line 6, in <module>
    import swh.web.api.views.content # noqa
  File "/home/antoine/swh/swh-environment/swh-web/swh/web/api/views/content.py", line 13, in <module>
    from swh.web.api.apidoc import api_doc, format_docstring
  File "/home/antoine/swh/swh-environment/swh-web/swh/web/api/apidoc.py", line 18, in <module>
    from rest_framework.decorators import api_view
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/decorators.py", line 13, in <module>
    from rest_framework.views import APIView
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/views.py", line 104, in <module>
    class APIView(View):
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/views.py", line 110, in APIView
    throttle_classes = api_settings.DEFAULT_THROTTLE_CLASSES
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/settings.py", line 220, in __getattr__
    val = perform_import(val, attr)
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/settings.py", line 168, in perform_import
    return [import_from_string(item, setting_name) for item in val]
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/settings.py", line 168, in <listcomp>
    return [import_from_string(item, setting_name) for item in val]
  File "/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/settings.py", line 180, in import_from_string
    raise ImportError(msg)
ImportError: Could not import 'swh.web.api.throttling.SwhWebRateThrottle' for API setting 'DEFAULT_THROTTLE_CLASSES'. ImportError: cannot import name 'APIView' from 'rest_framework.views' (/home/antoine/.virtualenvs/swh/lib/python3.7/site-packages/rest_framework/views.py)

I need to find another solution.

anlambert added inline comments.Mar 20 2020, 2:22 PM