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

aiohttp-cors dependency #1365

Closed
Alessandro-Barbieri opened this issue Apr 25, 2020 · 6 comments · Fixed by #2500
Closed

aiohttp-cors dependency #1365

Alessandro-Barbieri opened this issue Apr 25, 2020 · 6 comments · Fixed by #2500
Labels
C: dependencies T: bug Something isn't working

Comments

@Alessandro-Barbieri
Copy link

The aiohttp-cors dependency is problematic, currently it isn't python3.8 ready but black needs it to run tests.

@Alessandro-Barbieri Alessandro-Barbieri added the T: bug Something isn't working label Apr 25, 2020
@hugovk
Copy link
Contributor

hugovk commented Apr 25, 2020

What's the problem?

aiohttp-cors 0.7.0 is installed and used for Black's tests with Python 3.8:

Alessandro-Barbieri referenced this issue in gentoo/guru Apr 25, 2020
Package-Manager: Portage-2.3.99, Repoman-2.3.22
Signed-off-by: Alessandro Barbieri <lssndrbarbieri@gmail.com>
@Alessandro-Barbieri
Copy link
Author

The problem is that aiohttp-cors is unmaintained, doesn't officially support python3.7+ and have broken tests in python3.7+

@JelleZijlstra
Copy link
Collaborator

What's the concrete problem here? Our tests appear to pass with 3.8 and 3.9.

@Alessandro-Barbieri
Copy link
Author

@ichard26
Copy link
Collaborator

ichard26 commented Jun 26, 2021

Yes we understand that using a unmaintained dependency is a bad idea, but the thing is that it will be harder to convince us to invest the effort moving blackd to an alternative when to us, it's (or seems) fine. OK, aiohttp-cors's test suite doesn't pass Python 3.7 or 3.8, etc., but is there any concrete issues for users of blackd because of aiohttp-cors?

@cooperlees
Copy link
Collaborator

I’m all ears for a proposal to replace it with a maintained dependency and would accept a well tested PR.

Maybe we should write an integration test for the current implementation and then move to a new dependency to ensure minimal risk of missing a key element of functionality.

@ichard26 ichard26 linked a pull request Sep 19, 2021 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: dependencies T: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants