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

Update docs from Request.local to Request.ext #539

Merged
merged 1 commit into from
May 28, 2020
Merged

Update docs from Request.local to Request.ext #539

merged 1 commit into from
May 28, 2020

Conversation

bluk
Copy link
Contributor

@bluk bluk commented May 24, 2020

The method was renamed in Tide 0.9.

Just 2 cents, it was a bit confusing to me to use "local state" and "global state" as the names because IMHO those terms are normally associated with entire programs. Other frameworks call "local state" as "per request state" or "request scoped state". The "global state" is sometimes referred to as "application scoped state" as well. I'm not familiar with the design and terminology of this framework, but my suggestion would be to clarify/rename those concepts.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This is great, thanks heaps! I agree with your comments about "local/global state" being confusing. Changing it to what you suggested would be very welcome!

@bluk
Copy link
Contributor Author

bluk commented May 25, 2020

I added a commit to change the "local"/"global" state doc references since the first commit is trivial. Feel free to modify/discuss if needed.

Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

There's some mismatch here between Response::ext and Request::ext. As far as I can tell, both have extensions.

In addition to the below - https://github.com/http-rs/tide/blob/master/src/request.rs#L231-L240

src/lib.rs Show resolved Hide resolved
@yoshuawuyts yoshuawuyts mentioned this pull request May 28, 2020
@yoshuawuyts
Copy link
Member

@bluk I think CI is failing because of #531; I'm trying a fix in #552. Could you rebase on master once we merge? Thanks heaps!

- Clarify "local"/"global" state to request/response/application scoped
  state
@bluk
Copy link
Contributor Author

bluk commented May 28, 2020

Rebased and squashed to one commit. If there's anything else, please feel free to leave a comment.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This is fantastic, thanks heaps!

@yoshuawuyts yoshuawuyts merged commit fd2a545 into http-rs:master May 28, 2020
@bluk bluk deleted the update-local-to-ext branch May 28, 2020 20:40
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.

3 participants