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

🌐 Remove translation docs references to aiofiles as it's no longer needed since AnyIO #3594

Merged
merged 5 commits into from May 10, 2022

Conversation

alonme
Copy link
Contributor

@alonme alonme commented Jul 23, 2021

2022-05-09 Edit (by @tiangolo): the scope of this PR was changed to remove docs/translations references to aiofiles as the other intended changes were covered in other PRs.

Original description

Related to #3589

This Starlette PR (https://github.com/encode/starlette/pull/1158/files#) solves the issue from #3589.

Updating starlette to solve this issue in FastAPI

@alonme alonme changed the title Update starlette version Update starlette version - solves #3589 Jul 23, 2021
@alonme
Copy link
Contributor Author

alonme commented Jul 23, 2021

PR #3372 also updates starlette, however it seems to take a long time to merge as it includes many other changes.

I hope we can merge this smaller branch quicker to resolve #3589 .

I copied the test fix from #3372 to prevent conflicts later

If its easier for any reason - the same can be achieved by updating to 0.15.0

Copy link
Contributor

@graingert graingert left a comment

Choose a reason for hiding this comment

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

The docs about aiofiles will need removing

@alonme
Copy link
Contributor Author

alonme commented Jul 24, 2021

@graingert - Removed aiofiles and tested it still works for FileResponse and StaticFiles

@graingert
Copy link
Contributor

what do you think about the contextlib2 backport changes? Starlette depends on it on python3.6 so FastAPI doesn't need the async_generator or async_exit_stack deps anymore

@alonme
Copy link
Contributor Author

alonme commented Jul 24, 2021

@graingert - Not to familiar with these, aren't these more related to the trio/anyio change? can we remove the backports without changes from #3372 ?

@graingert
Copy link
Contributor

@alonme not directly related to anyio/trio no

@alonme
Copy link
Contributor Author

alonme commented Jul 24, 2021

@graingert - Thanks, backported these changes from the other PR.

@alonme
Copy link
Contributor Author

alonme commented Jul 27, 2021

Can anyone enable tests?

@Mause
Copy link
Contributor

Mause commented Jul 27, 2021

@tiangolo is the only person with merge and test run access unfortunately

@GuySpotnix
Copy link

+1
Please merge #3589

@alonme
Copy link
Contributor Author

alonme commented Aug 11, 2021

resolves #3674

@graingert
Copy link
Contributor

PR #3372 also updates starlette, however it seems to take a long time to merge as it includes many other changes.

I'm not sure which changes in #3372 would delay the PR

@alonme
Copy link
Contributor Author

alonme commented Aug 11, 2021

@graingert, me neither - it just seemed to me that it wasn't getting merge quickly at the time.
This pr was much smaller than #3372 , however after your rightful comments they aren't that different anymore.
This is still simpler, and i hoped it would be possible to merge it quickly as it fixes a bug which i think is pretty bad.

@graingert
Copy link
Contributor

I was considering adding wait_all to starlette at starlette.concurrency:wait_all as it's the most complicated bit of the changes imho

@alonme
Copy link
Contributor Author

alonme commented Aug 11, 2021

@tiangolo - Can we proceed with one of the PR's?

@graingert
Copy link
Contributor

It would be good to get a release with a new Starlette that supports py3.6-py3.10 before the December EOL of Python 3.6

@alonme
Copy link
Contributor Author

alonme commented Sep 22, 2021

@tiangolo - can we please go forward with this? or at least get a comment?
There are at least 2 issues that this will fix, and another project that waits for features from starlette 0.16

thanks

@brnosouza
Copy link

any update on this?

@stephan-hesselmann-by
Copy link

I have to say this is not a good look for the project. Not only this PR but many others are stalled with no comments from the maintainer. Actually it seems like the project is unmaintained.

@Mause
Copy link
Contributor

Mause commented Sep 27, 2021

@stephan-hesselmann-by you are correct unfortunately, this project is effectively abandoned

@billcrook
Copy link

@stephan-hesselmann-by you are correct unfortunately, this project is effectively abandoned

💯 I'm considering dropping down to starlette directly esp considering all the shims and overrides I'm putting into place to get everything how I want it (custom error responses, finer grained transaction control, singleton dependencies, #1359, etc...). API doc is nice I suppose?

@tiangolo
Copy link
Owner

The project is not abandoned at all.

In fact, one of the main things wanted in the existing PRs and issues was a way to improve how models are defined for databases and Pydantic, and support for the latest versions of SQLAlchemy, so I spent months building https://github.com/tiangolo/sqlmodel, which is made for FastAPI. 🚀


I have a lot of pending PRs to review and a lot of things in the backlog, and I'm covering them all gradually and with the minimum disruption possible. This also applies to all my Docker images, that for example, now support Python 3.9, all of them.

I'm carefully reviewing each PR and issue myself, and I have been changing my whole work-life in the last months, including dealing with visas and German bureaucracy to be able to dedicate much more time to all my open source projects.

Also, the comments claiming that it is abandoned were made 2 hours ago, when the last commit improving the HTTPS guide was done 3 hours ago. 🤦 😞

The improved HTTPS guide is part of an effort into improving the information about deployments and how (and when) to use which Docker images, etc. So that will come soon too. 🎉

@adriangb
Copy link
Contributor

Thank you for the response @tiangolo! I'm very glad this project is alive 😄

First off, I think it is amazing the amount and quality of work you are able to do as one person. That said, I do think it might be best for the health of the projects to build teams by giving repeat contributors that have shown to produce high quality work more privileges, that way they can get PRs into a good state (by providing feedback to PR authors) before you even have to look at them, even if you are still the one giving the final ✅ . This can also be codified, e.g. w/ CODEOWNERS files.

@v3ss0n
Copy link

v3ss0n commented Oct 2, 2021

The project is not abandoned at all.

In fact, one of the main things wanted in the existing PRs and issues was a way to improve how models are defined for databases and Pydantic, and support for the latest versions of SQLAlchemy, so I spent months building https://github.com/tiangolo/sqlmodel, which is made for FastAPI. rocket

I have a lot of pending PRs to review and a lot of things in the backlog, and I'm covering them all gradually and with the minimum disruption possible. This also applies to all my Docker images, that for example, now support Python 3.9, all of them.

I'm carefully reviewing each PR and issue myself, and I have been changing my whole work-life in the last months, including dealing with visas and German bureaucracy to be able to dedicate much more time to all my open source projects.

Also, the comments claiming that it is abandoned were made 2 hours ago, when the last commit improving the HTTPS guide was done 3 hours ago. facepalm disappointed

The improved HTTPS guide is part of an effort into improving the information about deployments and how (and when) to use which Docker images, etc. So that will come soon too. tada

Awesome to hear from you @tiangolo . I am using fastapi for almsot a year now. And the popularity keep rising.
I am sure FastAPI would be benefit from Community Driven development , since the project is growing faster for one person to manage, have you consider starting a foundation / organization and appointing good contributors to help you review code / merge?

That would make FastAPI grow bigger and FastAPI is really the web framework python deserves.

@brnosouza
Copy link

@alonme I think you need to rebase, mainly because a similar PR was merged and contains a few updates

@agronholm
Copy link

Does this PR add anything compared to the merged one?

@alonme
Copy link
Contributor Author

alonme commented Oct 7, 2021

@brnosouza - thanks, will get to that a bit later
@agronholm - from what i have seen, the update is only to starlette 0.15.0, and not 0.16.0

so my update also accounts for the aiofiles documentation @tiangolo described here #2899 (comment)

@alonme
Copy link
Contributor Author

alonme commented Oct 7, 2021

rebased

@alonme
Copy link
Contributor Author

alonme commented Oct 7, 2021

ok, now i see @tiangolo created his own PR to update to 0.16.0

So this only includes docs fix

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #3594 (6232e44) into master (8a353ab) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #3594   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          531       531           
  Lines        13629     13629           
=========================================
  Hits         13629     13629           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a353ab...6232e44. Read the comment docs.

@tiangolo tiangolo changed the title Update starlette version - solves #3589 🌐 Remove translation docs references to aiofiles as it's no longer needed since AnyIO May 10, 2022
@github-actions
Copy link
Contributor

📝 Docs preview for commit 6232e44 at: https://6279ba36fab0da14c006f463--fastapi.netlify.app

@tiangolo
Copy link
Owner

Thanks for the discussion everyone, this was solved a while ago in another PR and has been available for a while in recent releases.

Given that, I reduced the scope of this particular PR to remove the docs/translations references to aiofiles. Thanks for the interest and discussion you all! ☕

@tiangolo tiangolo merged commit c0d6865 into tiangolo:master May 10, 2022
JeanArhancet pushed a commit to JeanArhancet/fastapi that referenced this pull request Aug 20, 2022
…eded since AnyIO (tiangolo#3594)

Co-authored-by: AlonMenczer <alonm@spotnix.io>
Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
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