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: First draft of signals chapter #105

Merged
merged 6 commits into from
Dec 7, 2018

Conversation

killercup
Copy link
Collaborator

Closes #50
cc #27

@killercup
Copy link
Collaborator Author

r? @stjepang

I tried to keep it simple for now, and hope it's still correct. What more do I need to mention to make this really useful?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks really good, I like it!

I'm leaving a few nits inline.

src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals-channels.rs Outdated Show resolved Hide resolved
src/in-depth/signals.md Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 7, 2018

@vorner Just pinging you in case you'd like to take a look.

src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
src/in-depth/signals.md Outdated Show resolved Hide resolved
@killercup
Copy link
Collaborator Author

Thanks so much for the super fast reviews, @Screwtapello and @stjepang! <3

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

As far as I'm concerned, this PR is now good to go. 🚀

@killercup
Copy link
Collaborator Author

Let's land it! As always, comments here and on #58 are welcome even after the merge!

@killercup killercup merged commit 11c23e9 into rust-cli:master Dec 7, 2018
let ticks = tick(Duration::from_secs(1));

loop {
select! {
Copy link

Choose a reason for hiding this comment

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

select! is deprecated, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the select macro from crossbeam-channels, which I think is still going strong

Copy link

Choose a reason for hiding this comment

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

This is crossbeam_channel::select!, not std::select! (which is deprecated).

Copy link

Choose a reason for hiding this comment

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

ah, pardon me. :)

for example to close network connections,
remove temporary files,
or reset system settings,
this chapter is however very relevant!
Copy link

Choose a reason for hiding this comment

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

Is this a good place to point out one should still be able to cope with non-clean shutdown somehow (machine crash, sigkill, crash of program), like cleaning up the temporary files on the next start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that's very a interesting point! I can add a short paragraph here, but maybe it would make more sense to add a full chapter on "writing robust CLI applications". What do you think?

Copy link

Choose a reason for hiding this comment

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

The other chapter probably sounds better (and it probably is not specific to CLI only). Still, as a wording nitpick, maybe not call it „cleanup“, but „graceful shutdown“? Like, the OS will terminate network connections, but it might happen in the middle of a message instead of after saying proper „Good bye“.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Expanded the note to describe graceful shutdown scenarios, and opened #107 for tracking ideas around a "robustness" chapter.

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