Skip to content

deprecate RequestContext.g#4342

Merged
davidism merged 1 commit into
mainfrom
deprecate-req-ctx-g
Nov 16, 2021
Merged

deprecate RequestContext.g#4342
davidism merged 1 commit into
mainfrom
deprecate-req-ctx-g

Conversation

@davidism
Copy link
Copy Markdown
Member

@davidism davidism commented Nov 16, 2021

It was moved to AppContext.g a long time ago.

closes #3898

@davidism davidism added this to the 2.1.0 milestone Nov 16, 2021
@davidism davidism merged commit 9486b6c into main Nov 16, 2021
@davidism davidism deleted the deprecate-req-ctx-g branch November 16, 2021 15:38
@laggardkernel
Copy link
Copy Markdown
Contributor

laggardkernel commented Nov 17, 2021

@davidism The proposal started as a question from me at first. But after a 2nd thought. I guess we can just keep the property RequestContext.g.

If we follow the idea to access attributes only where it's stored. The .app should not kept on RequestContext neither. Obviously, we can't remove .app from RequestContext cause it's also used in other methods on RequestContext, like .push(). This makes me doubt my former proposal. And keeping .g makes RequestContext a handy place to access all the globals (app, g, request, session). I tried to start a discussion with you maintainers in the original issue but haven't got any reply yet, only find a PR with this change got merged today.

@davidism
Copy link
Copy Markdown
Member Author

g is not bound to the request context, it doesn't make sense as a property there. Additionally, the property always points at the top of the app context stack, not the context that was associated with the specific request context. I'm fine with the change, otherwise I wouldn't have made it.

@laggardkernel
Copy link
Copy Markdown
Contributor

Whether keeping g on RequestContext or not is totally okay for me. I couldn't make the decision before which is the reason I replied in the original issue.

the property always points at the top of the app context stack, not the context that was associated with the specific request context.

Thanks for pointing it out. I didn't consider the problem from this aspect.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deprecated and remove RequestContext.g, it is AppContext.g

2 participants