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

book: Extend main loop chapter with async section #1511

Merged
merged 16 commits into from
Oct 27, 2023

Conversation

Hofer-Julian
Copy link
Collaborator

@Hofer-Julian Hofer-Julian commented Oct 11, 2023

See discussion in #1508

  • Show how to await on gio::spawn_blocking
  • Await on ashpd
  • Use reqwest with tokio

@Hofer-Julian Hofer-Julian force-pushed the Hofer-Julian/book-async branch 4 times, most recently from 21f79cf to 142840b Compare October 22, 2023 10:59
@sdroege
Copy link
Member

sdroege commented Oct 22, 2023

Let me know when you want me to go over it for a review :)

@Hofer-Julian
Copy link
Collaborator Author

Let me know when you want me to go over it for a review :)

Thanks! Will do that :)

@Hofer-Julian Hofer-Julian force-pushed the Hofer-Julian/book-async branch from a99748b to c35484d Compare October 22, 2023 16:00
@Hofer-Julian
Copy link
Collaborator Author

@sdroege @felinira this PR is now ready for review. Feel free to also comment on parts of the chapter that wasn't touched by this PR.

You can render it locally by running:

cargo install mdbook
git switch Hofer-Julian/book-async
mdbook serve book

CI fails because I reference listings that will be there after this PR is merged.

@Hofer-Julian Hofer-Julian marked this pull request as ready for review October 22, 2023 16:41
@Hofer-Julian Hofer-Julian requested a review from sdroege October 22, 2023 16:41
Copy link
Contributor

@felinira felinira left a comment

Choose a reason for hiding this comment

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

I've not run the examples, but I've gone through everything and made some suggestions. Overall this is looking pretty good and is a useful addition I think :)

book/src/main_event_loop.md Outdated Show resolved Hide resolved
book/src/main_event_loop.md Outdated Show resolved Hide resolved
book/src/main_event_loop.md Outdated Show resolved Hide resolved
book/src/main_event_loop.md Outdated Show resolved Hide resolved
book/src/main_event_loop.md Outdated Show resolved Hide resolved
book/src/main_event_loop.md Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Thanks for the review @felinira!
Your suggestions make sense to me and I applied most of the immediately :)

book/src/main_event_loop.md Outdated Show resolved Hide resolved
@Hofer-Julian Hofer-Julian merged commit 776fa54 into master Oct 27, 2023
3 checks passed
@Hofer-Julian Hofer-Julian deleted the Hofer-Julian/book-async branch October 27, 2023 11:13
@sdroege
Copy link
Member

sdroege commented Oct 30, 2023

A bit late, but I went through everything now and don't have a single suggestion what to change. Good work, this is a lot more understandable and short than what I would've come up with :)

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.

4 participants