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

Upgrading the library: full typing support, remove deprecated methods #46

Closed
wants to merge 2 commits into from

Conversation

delvinru
Copy link
Contributor

Hi, I've been using your library for quite a long time and in many projects, thanks for the work!

When developing projects in python 3.12 with the IDE configured, the following errors often occur, which have to be closed using # type: ignore:

CleanShot 2024-12-19 at 9  28 25@2x

CleanShot 2024-12-19 at 9  28 41@2x

I decided to add more type support to the project, as well as get rid of deprecated methods. In addition, new versions of python-socks cause errors in these methods.

CleanShot 2024-12-19 at 9  33 10@2x

I ran tests, mypy and ruff, they all show that everything is ok.
CleanShot 2024-12-19 at 9  37 57@2x

If there are any comments, I am open to discussion and correction of errors.

@romis2012
Copy link
Owner

romis2012 commented Dec 20, 2024

First of all, I think we shouldn't remove the deprecated API until version 1.x is released. There are projects that still use it:
https://github.com/search?q=SocksConnector+language%3APython&type=code&p=2&l=Python

Secondly, there is no need to add type annotations to the parameters of "private" methods (_wrap_create_connection). Users of the aiohttp_socks package will not benefit from this.

Thirdly, there is no need to explicitly list all possible parameters in the signature of the __init__ method of the ProxyConnector class. These parameters have nothing to do with ProxyConnector. This complicates its maintenance.

Fourth, you do not need to import third-party modules (aiohappyeyeballs) that are not used in the ProxyConnector implementation.

Fifth, some type annotations appear to be incorrect (username: str = "" should be replaced with username: Optional[str] = None etc.)

@romis2012
Copy link
Owner

In general, from everything you suggested, I would leave only parameter type annotations for methods that are public API of the ProxyConnector class (__init__, from_url)

@delvinru
Copy link
Contributor Author

Take 1

I decided to look at projects that use the old API and got the following table:

repo last commit date comment
https://github.com/thinxer/tornado-proxy-server 13 years ago outdated package
https://github.com/HMaker/aiohttp-socks5/ 2 years ago outdated package
https://github.com/vpei/node/tree/main 1 year ago vendored?
https://github.com/ParrotSec/python-aiohttp-socks 5 years ago outdated package
https://github.com/maemo-leste-extras/aiohttp-socks/ 4 years ago fork/outdated package
https://github.com/rytilahti/aiohttp-socks/tree/master 5 years ago fork/outdated package
https://github.com/resource-not-found-blank/aiosocks/tree/master 8 years ago fork/outdated package
https://github.com/orleven/Tentacle/tree/master 9 month ago can be easily upgraded
https://github.com/AKKPP/ChessCoin032-Smartphone-Wallet-for-Android-and-iOS-/tree 2 years ago outdated package
https://github.com/NotSoSuper/NotSoBot/tree/master 7 years ago outdated package
https://github.com/proganalysis/python3_types/tree/master 4 years ago depends on aiosocks
https://github.com/aiogram/aiograph/tree/master 3 years ago outdated package / can be easily upgraded
https://github.com/StanGirard/TrollHunter/tree/master 4 years ago outdated package
https://github.com/Galarzaa90/tibia.py/tree/main 2 month ago still in usage, but can be easily upgraded
https://github.com/monolithworks/decaptcha-dm/tree/master 5 years ago outdated package
https://github.com/HyperionGray/starbelly/tree/master 4 years ago outdated package
https://github.com/sphericale/electrum-raven/tree/master 4 years ago fork
https://github.com/exploit-inters/CMS-Attack 6 years ago outdated package
https://github.com/s1g0day/ICP_Query_Batch/tree/main 4 month ago still in usage, but can be easily upgraded
https://github.com/shunf4/python-servlets/tree/master 2 month ago not actually used, just imported, in use - ProxyConnector
https://github.com/joyn-gg/EmoteManager/tree/master 2 years ago outdated package
https://github.com/wanghaisheng/keyword-genius/tree/main 4 month ago already used ProxyConnector in new version
https://github.com/Yakudzaki/KuzyaBot/tree/main 3 month ago not actually used, just imported
https://github.com/asynccnu/iproxy/tree/master 7 years ago outdated package
https://github.com/monokrome/slick/tree/master 6 years ago outdated package
https://github.com/AlJohri/python-tor-examples/tree/master 7 years ago outdated package

etc

As you can see, all packages using the old API themselves have long been outdated. In live projects, it can be easily changed to a ProxyConnector.
If you are interested in supporting such old projects, no problem, I will return the old API.

Take 2
I totally agree, I'll change it.

Take 3
I have doubts here, it makes it easy to throw parameters into the class, but I also understand your position with maintenance. I think I will also change it

Take 4
Because the library relies on aiohttp, which uses aiohappyeyeballs, I took the type annotation directly from the aiohttp.
I agree here, I'll change it

Take 5
This types and initialization was done on purpose, because the library uses python-socks, which expects not Optional[str], just str.

@romis2012
Copy link
Owner

Take 1

You are not taking into account projects that are not hosted on github
...

Take 5
...because the library uses python-socks, which expects not Optional[str], just str

It is python-socks mistake, which should be fixed later. A public API should not depend on implementation details.
For the same reason, please import SSLContext from ssl module directly (not from aiohttp.connector).

And please remove cast call from from_urls method body, it would be better to add type annotations to python_socks.parse_proxy_url later.

@romis2012
Copy link
Owner

And return the deprecated APIs

@delvinru
Copy link
Contributor Author

You are not taking into account projects that are not hosted on github

Of course not, because i can't review them)

Ok, I'll just reopen the pull request with minimal changes.

@delvinru delvinru closed this Dec 20, 2024
@delvinru delvinru mentioned this pull request Dec 20, 2024
@delvinru delvinru deleted the add-more-typing branch December 27, 2024 12:07
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