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

Fix linting errors #95

Closed
wants to merge 1 commit into from
Closed

Conversation

mgax
Copy link
Contributor

@mgax mgax commented Aug 1, 2024

The CI currently fails because a new version of Mypy is raising new errors.

@@ -67,4 +67,4 @@ def create_connection(self, alias: str) -> BaseTaskBackend:

tasks = TasksHandler()

default_task_backend = ConnectionProxy(cast(Mapping, tasks), DEFAULT_TASK_BACKEND_ALIAS)
default_task_backend = cast(Any, ConnectionProxy(tasks, DEFAULT_TASK_BACKEND_ALIAS))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super happy with casting default_task_backend to Any, but casting it to BaseTaskBackend results in dozens of Mypy errors in tests, and this would have been a much larger PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the new errors correct errors, or is something else happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are correct, because the tests know they are referencing a DummyBackend, which has extra methods, like clear. Here's the output of just lint: https://gist.github.com/mgax/e2c1c1e603c5700209571ae4ebf04a43.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the error this was causing before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from CI:

django_tasks/__init__.py:70: error: Need type annotation for "default_task_backend"  [var-annotated]
django_tasks/__init__.py:70: error: Argument 1 to "ConnectionProxy" has incompatible type "Mapping[Any, Any]"; expected "BaseConnectionHandler[Never]"  [arg-type]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the ConnectionProxy types aren't quite right.

That said, I still don't think setting a public type to Any is a good idea. BaseTaskBackend is the correct type - the other uses just need their type ignored.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is likely typeddjango/django-stubs#2246, where the types were previously Any, which is why they passed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've attempted to fix this upstream: typeddjango/django-stubs#2311

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was quick!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like the proper fix will involve a bit of work. This issue is currently blocking the CI on django-tasks. I suggest that we merge this simple fix and open a ticket to properly type default_task_backend later.

@mgax mgax marked this pull request as ready for review August 1, 2024 20:26
@RealOrangeOne
Copy link
Owner

Fixed in 63fcda1

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