[DOC] Fix example usage of get_context (plus a little more) #801
+5
−4
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Fixes: #793
NOTE: Sorry, I'm not sure what the target branch for this PR is supposed to be, I thought I initially branched off of v1.9.1, but that doesn't appear to be a branch, i can rebase it to v1.7.x, where it looks like the
get_context()
issue was already fixed... but the README doesn't reflect this...The example code in the readme contains a line that invokes
server.get_context()
on the lowlevel server. There is no such method, the proper way to access the context is via the property-decorated functionserver.request_context
.This apparently was just raised in #799 as well. However, I think the issue is a bit more problematic. Forgive me if I'm getting it wrong.
The example code for the lowlevel server was corrected here and in #799 to avoid
get_context()
. But this is in fact a method on FastMCP() - but this is not actually illustrated in the code examples(!). In the README, the part on FastMCP usesrequest_context
:So:
get_context()
request_context
and there is no actual example of correct usage ofget_context()
So this is very odd.
It seems to me that if anything, the convenience method that FastMCP offers that the lowlevel server lacks is
get_context()
, so this ought to be the thing that's documented in the README. What's worse is I don't really see what advantage there is in usingget_context()
vs. passing in thectx: Context
parameter. Because if you useget_context()
, you still have to access.request_context
e.g. if you want the lifespan, you need to doapp.get_context().request_context.lifespan_context
, which is equivalent toctx.request_context.lifespan_context
. So, again my apologies if I am the one confused, but I think there is either some redundancy in the way these things are accessed, or,get_context()
should really be directly returningctx.request_context
. Then you could do something likelifespan = app.get_context().lifespan_context
.In my PR, besides fixing the incorrect invocation of
get_context
I add an example that illustrates correct usage ofget_context
for FastMCP.All the being said, given the existence of the
get_context
method, I'm struggling a bit to see what reason or convenience there is in using thectx: Context
parameter to access the context instead of justmy_app.get_context()
.How Has This Been Tested?
All tests/list/etc. are passing.
Breaking Changes
No.
Types of changes
Checklist
Additional context
It seems to me that the API for FastMCP should be simplified, I don't see a good reason to have two different "convenience" ways to get the same result, and it strikes me as confusing. I don't personally have a preference for one or the other. Alternatively,
get_context
could be refactored to return thectx.request_context
itself, which would be some kind of differentiator at least.