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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 1.0.0 #1888

Closed
wants to merge 15 commits into from
Closed

Version 1.0.0 #1888

wants to merge 15 commits into from

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Oct 3, 2022

Description

This PR bumps Starlette to 1.0.0. 馃帀

Changes

  • Bump Starlette to 1.0.0.
  • Add deprecation policy (docs/deprecation.md).
  • Remove all code that was marked as deprecated.
  • Deprecate BaseHTTPMiddleware.
  • Deprecate decorator functions in the Router method, which were discouraged for some time.

Note
UPDATE [05/10/22]: PRs created according to comments on this PR:

Blockers

A blocker is PR or idea that can potentially make us jump to 2.0.0 too fast. If you see one around, please comment.

Note
UPDATE [22/10/22]: We need to fix the python-multpart issues.

Missing

  • Deprecate decorators on Starlette - I've only deprecated on Router until now.

Note
UPDATE [05/10/22]: Based on the comments on this PR, there's an agreement that Starlette 1.0 will not have deprecation warnings. We shall remove all deprecated code before that.

Questions

  1. Should we remove the code around the lifespan warnings? Is that part of the code supposed to be removed?
  2. Should we remove the allow_redirects deprecation? It was recently added.
    • Yes. It's been months already.
  3. Should we think about release cycle?
    • We are going to support only the latest version.

How to review this PR?

  1. Check warnings.warn in code.
  2. Read the Deprecation Policy section using ./scripts/docs serve.
  3. Try hyperlinks used in the code, and release notes.

@Kludex Kludex force-pushed the release/1.0.0 branch 2 times, most recently from 0ad917c to c7d5364 Compare October 3, 2022 18:37
@Kludex Kludex mentioned this pull request Oct 3, 2022
@alex-oleshkevich
Copy link
Member

This is good news! Here are my thoughts about deprecation.

As Starlette goes 1.0 then it is a good time to drop all deprecated stuff and apply breaking changes. If this is too risky at this moment, then I would advise rolling out a few more 0.xx releases with deprecation warnings and then release 1.0 by the end of the year.

@alex-oleshkevich
Copy link
Member

We may want to get back to #1803

Copy link
Sponsor Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Awesome! Exciting! I just have a couple of minor comments.

docs/deprecation.md Show resolved Hide resolved
docs/deprecation.md Show resolved Hide resolved
docs/release-notes.md Outdated Show resolved Hide resolved
starlette/status.py Show resolved Hide resolved
@tiangolo
Copy link
Sponsor Member

tiangolo commented Oct 4, 2022

Another thing to have in mind that might be worth deprecating and can create some strange behaviors/issues now: app.add_middleware would create multiple instances of any previous middlewares as described here: #1161

There are not many cases where this is a problem, but it is in some, for example when dealing with Prometheus, that expects that an object is created and registered only once.


I was chatting with @Kludex and he pointed me to that discussion, so, mentioning it here as well.

@Kludex Kludex added this to the Version 1.0 milestone Oct 4, 2022
@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 4, 2022

I also agree on what @alex-oleshkevich (if I got what you said right) and @tiangolo said about the deprecation.

The plan changes a bit then, we should include the deprecation warnings on the next release (0.22.0).

Then, on this PR, we remove all the deprecation warnings - Let's not have any pendency on our stable release. Conceptually, it makes more sense to do this.

@tiangolo
Copy link
Sponsor Member

tiangolo commented Oct 5, 2022

Last night I remembered a couple of extra things I had pending and probably worth discussing here.

Closing UploadFile

When enabling tests for Python 3.11 in FastAPI, I'm getting errors about UploadFile not being closed. It seems Python 3.10 and below don't complain about that. But it would make sense that it would expect those files to be closed.

It seems those errors don't show up in Starlette's tests for Python 3.11, but maybe it's just because that code path is currently not tested, not sure.

The other thing is, it might be the decision in Starlette that users/developers are responsible for closing those UploadFile objects or something, in that case, I should just handle that in FastAPI, but I wanted to mention it here in case it's something that should be fixed.

I know this should be a proper issue/example. But I thought it was worth at least mentioning it here before the changes and releases are fixed.

Routers supporting empty trailing slash

What I refer with this is that there could be a router that handles all the routes under some prefix, for example: https://github.com/encode, and it would also handle https://github.com/encode/starlette and https://github.com/encode/uvicorn. But the same router handles https://github.com/encode without requiring it to end in a trailing slash. BTW, these GitHub URLs work like that.

Currently, Starlette doesn't internally support that, at least not in the same router.

FastAPI has supported that in a strange way, because in FastAPI, when using app.include_router(), currently it just copies all the paths, and in the end, it's just a single global router with everything, in FastAPI routers are currently not really "mounted" as in Starlette. So, if a router has a route with a path of "", (instead of "/"), that would handle that top level route without the trailing slash.

Now, I've wanted to refactor that in FastAPI, so that routes are not copied but routers are actually mounted (or something similar). This will allow some things on the FastAPI side. But one of the blockers I found was that I couldn't use the standard Starlette Routers and Mounts because that would not support prefixes without a trailing slash.

I need to support that and a couple of other things in FastAPI.

I'm totally fine if the decision is to not support that in Starlette, but I thought it was worth mentioning that here too, in case it seems it's something that would make sense to support.

@adriangb
Copy link
Member

adriangb commented Oct 5, 2022

Routers supporting empty trailing slash

Thank you for bringing that up Sebasti谩n. I've wanted to fix that for a while (I wrote the implementation at one point) but it required some minor breaking changes (I think it had to do with what errors are raised). If we want to make the change I'm happy to make the PR.

@Kludex Kludex force-pushed the release/1.0.0 branch 2 times, most recently from 97eb17a to 9ab3b84 Compare October 5, 2022 20:47
@adriangb
Copy link
Member

adriangb commented Oct 5, 2022

With respect to UploadFile: can you add the .close() to the AsyncExitStack for dependencies? It works in Xpresso: https://github.com/adriangb/xpresso/blob/ae1586513b8ee48bfe9a8fdb2e7cc7038c37053d/xpresso/binders/_binders/file_body.py#L76

@Kludex
Copy link
Sponsor Member Author

Kludex commented Feb 6, 2023

Another thing to have in mind that might be worth deprecating and can create some strange behaviors/issues now: app.add_middleware would create multiple instances of any previous middlewares as described here: #1161

There are not many cases where this is a problem, but it is in some, for example when dealing with Prometheus, that expects that an object is created and registered only once.

I was chatting with @Kludex and he pointed me to that discussion, so, mentioning it here as well.

app.add_middleware doesn't have any issues since 0.24.0 thanks to @adriangb. 馃檹

@Kludex Kludex mentioned this pull request Feb 14, 2023
8 tasks
@adriangb
Copy link
Member

adriangb commented Mar 10, 2023

Let鈥檚 add #2068

@Kludex
Copy link
Sponsor Member Author

Kludex commented Mar 16, 2023

I'm going to review both (which I also invite others to review):

After a decision is made there, let's release the last zero version release, then wait some weeks, and then prepare for 1.0 with:

Comments, feedback, or anything that you want to include before 1.0, please feel free to talk cc @encode/maintainers

@Kludex Kludex force-pushed the release/1.0.0 branch 2 times, most recently from d20b80b to 04bb396 Compare March 20, 2023 08:18
@BrettMoan
Copy link

BrettMoan commented Apr 25, 2023

I'm just here to write a thank you to all the people here supporting starlette and fastapi and other related repos. And congratulate you on a massive undertaking (an official release you are feeling prod and confident of)

Often open source is thankless and while I may be a random person on the internet you don't know, know I'm a person out there like you, typing away as part of a living supporting my family via a career in programming. Using starlette and fastapi has had helped me tremendously at my job as a backend developer. It has been a joy to work with.

(this includes pydantic as well where theres also a decent amount of contributor overlap)

Thank you. Really. My job wouldn't be nearly as sane without all the of the great work y'all have already done, both in implementation and documentation.

@Kludex
Copy link
Sponsor Member Author

Kludex commented Apr 25, 2023

@BrettMoan Thank you for saying it. Much appreciated! 鉂わ笍

@Kludex
Copy link
Sponsor Member Author

Kludex commented Dec 23, 2023

Let's go to #2384.

I've cleaned the branch. There were too many conflicts, I thought it would be easier to create a new one.

@Kludex Kludex closed this Dec 23, 2023
@Kludex Kludex deleted the release/1.0.0 branch December 23, 2023 09:03
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.

Version 1.0
5 participants