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

📝 Rename Websocket by WebSocket #272

Merged
merged 6 commits into from Jun 27, 2021
Merged

📝 Rename Websocket by WebSocket #272

merged 6 commits into from Jun 27, 2021

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Jun 25, 2021

This is fundamentally unnecessary, so the rejecting this PR is understandable. But the correct name is WebSocket.

This is fundamentally unnecessary, so the rejecting this PR is understandable. But the correct name is WebSocket.
@andrewgodwin
Copy link
Member

I don't object to it, but if you want to land this you'll need to do it in a backwards-compatible fashion (so the old names work but raise a warning)

@Kludex
Copy link
Contributor Author

Kludex commented Jun 26, 2021

I've used PEP 562 idea here, but as it was introduced in 3.7, so I added the backport implementation for 3.6 as requirement. I can also copy the file if you wish so, and we don't add it as dependency.

I've also added tests to check that the warning is being raised on the wanted types.

@andrewgodwin
Copy link
Member

I'd rather just do it the "old fashioned" way if we could - expose them as actual top-level module objects rather than bringing in extra code just for this.

@Kludex
Copy link
Contributor Author

Kludex commented Jun 26, 2021

I'm not following you. 🤔 How am I supposed to warn the user about the deprecation at import time without this implementation?

Or you just meant to not use __all__ and __deprecated__?

@andrewgodwin
Copy link
Member

Oh, I forgot they were type classes for a second, sorry. Thought they were actual classes where you could do it with a mixin on instantiation.

In that case, let's vendor pep562 in provided the license matches - I don't want more dependencies!

self._get_dir: Optional[Callable[..., List[str]]] = getattr(
self._module, "__dir__", None
)
sys.modules[name] = self # type: ignore[assignment]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sys.modules expects a Module and self is a Pep562, jfyk.

@Kludex
Copy link
Contributor Author

Kludex commented Jun 26, 2021

There's a mismatch between the formatters, isort and black are reformatting themselves on the pre-commit 🤔

EDIT: black has by default 88 line length, but flake8 and isort are configured to have 119. On the pre-commit, isort runs and formats the code to 119, then black reformats back to 88. 😅

If I apply either configuration, I'll have a lot more diffs here... Should I open another PR to fix this mismatch, and which one do you prefer, 88 or 119?

EDIT2: The problem is with this big import:

from typing import (
    Any,
    Awaitable,
    Callable,
    Dict,
    Iterable,
    List,
    Optional,
    Tuple,
    Type,
    Union,
)

Which isort says it should be in the same line, and black says it should not. 😅

@Kludex
Copy link
Contributor Author

Kludex commented Jun 26, 2021

Notes about the changes:

  • I've added pep562 as private module, so we can remove easily by the 3.6 EOL.
  • I've removed all the unnecessary code on _pep562.py (and tried to modify the less I could, jfyk).

@andrewgodwin
Copy link
Member

I think sticking with black's default length of 88 is the best approach - let's handle that in a separate PR. I'll merge this in now so we can avoid mixing linting in here - I think it looks good now, so thanks for bearing with my requests!

@andrewgodwin andrewgodwin merged commit 2843f67 into django:main Jun 27, 2021
@Kludex
Copy link
Contributor Author

Kludex commented Jun 27, 2021

Thank you! 🎉

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

2 participants