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

ExitCode::exit_process() method #95356

Merged
merged 4 commits into from
May 13, 2022

Conversation

coolreader18
Copy link
Contributor

@coolreader18 coolreader18 commented Mar 27, 2022

cc @yaahc / #93840

(eeek, hit ctrl-enter before I meant to and right after realizing the branch name was wrong. oh, well)

I feel like it makes sense to have the exit(ExitCode) function as a method or at least associated function on ExitCode, but maybe that would hurt discoverability? Probably not as much if it's at the top of the process::exit() documentation or something, but idk. Also very unsure about the name, I'd like something that communicates that you are exiting with this ExitCode, but with a method name being postfix it doesn't seem to flow. code.exit_process_with() ? .exit_process_with_self() ? Blech. Maybe it doesn't matter, since ideally just code.exit() or something would be clear simply by the name and single parameter but 🤷

Also I'd like to touch up the ExitCode docs (which I did a bit here), but that would probably be good in a separate PR, right? Since I think the beta deadline is coming up.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 27, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2022
@coolreader18 coolreader18 force-pushed the exitstatus-exit-method branch 2 times, most recently from 96a75a3 to 34cc4e7 Compare March 27, 2022 06:14
@coolreader18
Copy link
Contributor Author

Made #95389, I'll rebase onto that.

@yaahc yaahc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2022
library/std/src/process.rs Outdated Show resolved Hide resolved
@coolreader18
Copy link
Contributor Author

coolreader18 commented Mar 31, 2022

Also was wondering about an examples section; I'm not sure what a good sample use-case would be that wouldn't just be better served by a panic, my current idea is:

use std::process::Termination;
fn unwrap_or_exit<T, E: Debug>(res: Result<T, E>) -> T {
    match res {
        Ok(v) => v,
        Err(e) => {
            eprintln!("very fatal error:");
            Err::<Infallible, _>(e).report().exit_process()
        }
    }
}

But eehhhh. Do you have any ideas, or is it fine without? process::exit doesn't have an examples section, but it does have a fn main() { process::exit(match run_app() { /* essentially Result::report */ }) } which now that I'm thinking about it should probably be removed altogether, since that was just a stand-in for real return-exitcode-from-main functionality. But I wanted to have an example of exit_process that didn't encourage recreating that when you could just return result from main, but it probably is fine without it if that exit() example gets removed.

@coolreader18
Copy link
Contributor Author

coolreader18 commented Mar 31, 2022

But that's probably involved with the whole "what to do with process::exit() now" general question.

@coolreader18
Copy link
Contributor Author

Pushed a change to the process::exit() docs as well, just as an idea of what they could/should be.

@yaahc
Copy link
Member

yaahc commented Mar 31, 2022

Also was wondering about an examples section; I'm not sure what a good sample use-case would be that wouldn't just be better served by a panic, my current idea is:

use std::process::Termination;
fn unwrap_or_exit<T, E: Debug>(res: Result<T, E>) -> T {
    match res {
        Ok(v) => v,
        Err(e) => {
            eprintln!("very fatal error:");
            Err::<Infallible, _>(e).report().exit_process()
        }
    }
}

But eehhhh. Do you have any ideas, or is it fine without? process::exit doesn't have an examples section, but it does have a fn main() { process::exit(match run_app() { /* essentially Result::report */ }) } which now that I'm thinking about it should probably be removed altogether, since that was just a stand-in for real return-exitcode-from-main functionality. But I wanted to have an example of exit_process that didn't encourage recreating that when you could just return result from main, but it probably is fine without it if that exit() example gets removed.

I like the snippet inside of the example but yea, unwrap_or_exit is questionable. It's getting late so I didn't dig into this too deeply but my thought was to look at examples of ppl using process::exit for inspiration.

https://cs.github.com/?scopeName=All+repos&scope=&q=language%3Arust+process%3A%3Aexit

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2022
@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2022
@Dylan-DPC
Copy link
Member

@coolreader18 if you can update this with the latest changes as per the last comment we can push this forward

@rust-log-analyzer

This comment has been minimized.

library/std/src/process.rs Outdated Show resolved Hide resolved
library/std/src/process.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@yaahc
Copy link
Member

yaahc commented May 12, 2022

this LGTM once the todo is fixed. After you've fixed that up and it's passing the github checks go ahead and r=me

@bors delegate+

@bors
Copy link
Contributor

bors commented May 12, 2022

✌️ @coolreader18 can now approve this pull request

@bors
Copy link
Contributor

bors commented May 13, 2022

⌛ Trying commit a9e29d2 with merge 9ad4bde...

@coolreader18
Copy link
Contributor Author

ah shoot I told bors to try but I don't think that's what you meant, hopefully that's fine

@yaahc
Copy link
Member

yaahc commented May 13, 2022

ah shoot I told bors to try but I don't think that's what you meant, hopefully that's fine

did you delete the comment after? I don't see a bors command anywhere. The correct command should be @bors r=yaahc

@coolreader18
Copy link
Contributor Author

ha, yeah, I did delete it cause I thought maybe it would cancel it. anyway,
@bors r=yaahc

@bors
Copy link
Contributor

bors commented May 13, 2022

📌 Commit a9e29d2 has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2022
@coolreader18
Copy link
Contributor Author

One day I'll stop overthinking whether I'm using bors correctly, but I guess that's not today 🙃

@bors
Copy link
Contributor

bors commented May 13, 2022

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 9ad4bde to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 13, 2022
@bors bors merged commit 9ad4bde into rust-lang:master May 13, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 13, 2022
@yaahc
Copy link
Member

yaahc commented May 13, 2022

One day I'll stop overthinking whether I'm using bors correctly, but I guess that's not today 🙃

I use saved replies to help me with this, lol, and even then I often end up being confused, which reminds me, I meant to add a new saved reply for bors delegate, I had to look that up before #95356 (comment)

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9ad4bde): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@coolreader18 coolreader18 deleted the exitstatus-exit-method branch May 13, 2022 21:19
@yaahc
Copy link
Member

yaahc commented May 16, 2022

@coolreader18 I just realized I forgot an important step of the process. This method needs a tracking issue. Can you set one up and submit another PR to update the issue field to point to the new tracking issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants