8000 Improve sub-context passing ergonomics by nobbele · Pull Request #1037 · ggez/ggez · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve sub-context passing ergonomics #1037

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 31, 2022
Merged

Improve sub-context passing ergonomics #1037

merged 7 commits into from
May 31, 2022

Conversation

nobbele
Copy link
Member
@nobbele nobbele commented May 27, 2022

this allows passing just ctx to functions that take sub-contexts.
You can still split borrow with a slightly uglier construction: &(&ctx.fs, &ctx.gfx).
I think something like 99.9% of users won't ever have to split borrow so in my opinion, this is a good trade-off.

@nobbele nobbele requested a review from PSteinhaus May 27, 2022 22:45
@PSteinhaus
Copy link
Member

Interesting idea! It certainly improves ergonomics.
Two minor things:

  1. Why did you call the context parameter in the HasTwo cases extra? Wouldn't something like contexts be more meaningful?
  2. Do you think we should document how to split-borrow the context in some kind of example? I think you're right and most people won't ever use it, but I can imagine that there'll be enough people who wonder why this is the way it is and how the intended use it. I think the best way would be to shortly summarize the idea behind it in the documentation of Has and HasTwo, including a short code example on how to operate when splitting the context.

It certainly is a bit ugly, but for a library of our size and ambition (i.e. making life as easy as possible) it makes sense to me.

@nobbele
Copy link
Member Author
nobbele commented May 29, 2022

Why did you call the context parameter in the HasTwo cases extra? Wouldn't something like contexts be more meaningful?

fair, or ctxs or just ctx

Do you think we should document how to split-borrow the context in some kind of example?

sure, I'll write some documentation tomorrow

nobbele and others added 5 commits May 30, 2022 22:57
@nobbele
Rename extra to ctxs
bf2d5f9
@nobbele
Adds some documentation to Has traits
60a8fb7
@nobbele
Renames the Has getter to avoid name collisions and adds some docum…
3f0ac9f 
…entation
@nobbele
Adds some explanation to the FAQ
51349b5
@PSteinhaus
changed Has docs a little;
4b324b3 
changed the last two public functions taking references to GraphicsContext towards using Has and HasMut;
fixed clippy lints;
resolved failing tests (except skeptic for some reason);
PSteinhaus
PSteinhaus requested changes May 31, 2022
View reviewed changes
Copy link
Member
@PSteinhaus PSteinhaus left a comment

Choose a reason for hiding this comment

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

Almost finished. I found some final places where the Has-pattern hadn't yet been adopted and also changed some tiny other things, see the PR.

@nobbele nobbele requested a review from PSteinhaus May 31, 2022 19:09
@PSteinhaus PSteinhaus merged commit 6f90a16 into ggez:devel May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PSteinhaus PSteinhaus PSteinhaus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nobbele @PSteinhaus
0