feat: Add module for showing exit codes of shell pipeline #370

nrabulinski · 2019-09-14T13:46:02Z

Description

I implemented status module which can show the exit code of the last command or all exit codes of the last pipeline

Motivation and Context

Closes #193
Closes #882

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

Default configuration:

Symbols for exit codes:

No pipeline:

How Has This Been Tested?

I have tested using MacOS
I have tested using Linux
I have tested using Windows

Checklist:

I have updated the documentation accordingly.
I have updated the tests accordingly.

nrabulinski · 2019-09-14T15:13:10Z

Oh no, I forgot about the quirk @lilyball talked about - when using ! true the exit code should be 1 even though pipestatus returns 0.
Will fix in the next commit.

nickwb · 2019-09-15T03:45:42Z

I must admit, I'm a little confused by all of the configuration options - hopefully some new documentation could help with that? 🙂

This is just a thought - do you think there's value in modelling the display mode in an enum?

For example:

enum DisplayMode {
    ShowPipeline,              // Always show the pipeline
    ShowPipelineOnError,       // Show the pipeline on error, otherwise nothing
    ShowLastOrPipelineOnError, // Show the pipeline on error, otherwise the last status
    ShowLast,                  // Always show the last status
    ShowLastOnError            // Show the last status on error, otherwise nothing
}

jletey

Thanks for taking the time to work on this @nrabulinski 🎉!

Everything looks great!

I just have one request: Please take a look at #367, and do the same here. It would be nice to be able to change the delimiters surrounding the exit codes.

nrabulinski · 2019-09-15T16:30:33Z

I must admit, I'm a little confused by all of the configuration options - hopefully some new documentation could help with that? slightly_smiling_face

This is just a thought - do you think there's value in modelling the display mode in an enum?

For example:
enum DisplayMode {
    ShowPipeline,              // Always show the pipeline
    ShowPipelineOnError,       // Show the pipeline on error, otherwise nothing
    ShowLastOrPipelineOnError, // Show the pipeline on error, otherwise the last status
    ShowLast,                  // Always show the last status
    ShowLastOnError            // Show the last status on error, otherwise nothing
}

I've already talked about it in the Discord server, but I don't personally think there's sense to add ShowLastOrPipelineOnError. Other than that I think it was a great idea and I knew I had to change all those bools anyway. I don't know however if the way I solved it is the best/most intuitive one out there, so if you have any other suggestions - I'm all ears!

and a few smaller fixes one being changing delimeter_... to prefix/suffix as in starship#367

lilyball · 2019-09-16T04:24:05Z

Judging from the name, ShowLastOrPipelineOnError matches the behavior I use personally.

nrabulinski · 2019-09-16T06:22:24Z

@lilyball I just don’t see how it’d be different from my current OnError other than it’d show 0 each time there’s no error which isn’t very helpful. Although if it is something that people want, I can implement it.

lilyball · 2019-09-17T00:23:31Z

Skimming the code, it looks like what you've actually implemented doesn't do what I thought it did. In fact, it doesn't even implement the behavior of showing an error on ! false, because that has a pipestatus length of 1 and errored is false.

nrabulinski · 2019-09-17T08:42:51Z

@lilyball
And it’s only natural that a negated error is considered a success, isn’t it?
As I mentioned earlier - I don’t see the point of adding the mode you’re talking about since the only way it differs from Always is that it’d show 0 when there was no error instead of potentially whole pipeline.

It’d probably be much more viable to iterate through all the status codes in pipeline and show them when at least one of them errored which I might add with the next commit but as it is and considering what (at least I think) you’re talking about - it doesn’t make sense to add.

nkoehring · 2019-09-17T20:20:30Z

I'm new to the code of this project so I don't really know how other modules are doing it, but for the sake of readability I would suggest to move the default values (for example in line 57, line 59, line 60 and others) into constants in (or close to) the beginning of status.rs.

lilyball · 2019-09-19T05:50:50Z

@nrabulinski The whole point of my bash prompt status code setup is to convey the results of the previous job without extraneous info. The results include answering the question "is the status different from the pipestatus?"

So basically, my prompt shows pipestatus, unless it's all zeroes. If pipestatus disagrees with status, it shows that disagreement parenthesized. And it colors the output accordingly, so ! false shows an output in grey of [1 (0)]; the 1 because it's nonzero, and the (0) because that's the status diasgreeing with pipestatus. But ! true shows output in red of [(1)], pipestatus is omitted since it's all zeroes, but status disagrees so it's shown. This same output works for ! true | true as well, since again pipestatus is all zeroes. But e.g. ! false | false is (in grey) [1 1 (0)]. And ! false | true is (in red) [1 0 (1)]; the pipestatus is shown here because it has a nonzero component.

Ultimately, I'm left with the shortest prompt that doesn't discard any status info. I can tell at a glance what both $? and $PIPESTATUS are. And uninteresting output is omitted so I don't have to parse it just to find out that it's boring.

chipbuster · 2019-09-25T23:09:48Z

@nrabulinski Do you plan to make more changes to this PR or are we good to review as-is?

nrabulinski · 2019-09-26T06:41:54Z

@chipbuster I'll commit the changes @lilyball suggested in a day or two and then it should be ready to review

matchai

Looks good to me! 👍
Just seems you need to rerun the formatter.
If you can change the module to be disabled by default, we can go ahead and merge this.

Requesting review from another astronaut since this is a pretty big PR.

matchai · 2019-12-24T03:53:56Z

docs/config/README.md

+| `success_symbol`  | "✔"            | The symbol used if the exit code is equal to 0.                                                  |
+| `error_symbol`    | "✖"            | The symbol used if the exit code is non-zero.                                                    |
+| `simple_pipeline` | true           | Show only one exit code if all values in the pipeline are equal.                                 |
+| `disabled`        | false          | Disables the `status` module.                                                                    |


Thinking it over, I think we should keep this module disabled by default.
Could you please the configuration to reflect that?

matchai · 2019-12-24T03:58:27Z

src/modules/status.rs

+        None => vec![&exit_code],
+    };
+
+    // kind of a hack to not show `<previous status code>` when a user sends ^C to clear the line


matchai · 2019-12-24T03:59:29Z

src/modules/status.rs

+    let pipeline_error = pipestatus.iter().any(|&code| code != "0");
+    let mismatch = exit_code != *pipestatus.last()?;
+
+    let display_mode = status_config.display_mode; //get_display_mode(&module);


Is this comment meant to be in the PR?

matchai · 2019-12-24T04:01:27Z

tests/testsuite/status.rs

@@ -0,0 +1,209 @@
+use ansi_term::Color;


Tests look great! 👍

chipbuster · 2019-12-29T04:02:34Z

Looks like rustfmt failed on....whitespace? Not entirely sure what happened there but hopefully it's easy to fix.

lilyball · 2019-12-29T05:10:16Z

It looks like the empty line has 2 stray spaces in it, which rustfmt wants to delete.

jletey · 2020-01-12T07:49:30Z

Looks like rustfmt failed on....whitespace

I am still utterly confused by the fix 😅, but all tests are passing now!

matchai · 2020-01-12T22:52:56Z

@nrabulinski A friendly reminder about those review comments. ☝️

Lucretiel · 2020-01-27T00:27:16Z

src/configs/status.rs

+            "always" => DisplayMode::Always,
+            "error" => DisplayMode::OnExitError,
+            "any error" => DisplayMode::OnError,
+            /*"mismatch" |*/ _ => DisplayMode::OnErrorOrMismatch,


Recommend explicitly matching "mismatch" here, and panicking if it doesn't match one of those strings. Worst case, the panic is unreachable otherwise.

Lucretiel · 2020-01-27T00:30:27Z

src/main.rs

+    let pipestatus_arg = Arg::with_name("pipestatus")
+        .long("pipestatus")
+        .value_name("PIPESTATUS")
+        .help("Status codes from the previous pipeline")


Recommend adding a long_help here that documents the specific argument we're looking for. Is it a single, space-separated argument? Several individual arguments? Something else? If possible, I'd consider using Arg::multiple, which means that the expected format would be: --pipestatus 0 1 0 2 (as opposed to --pipestatus "0 1 0 2").

Ah, that’s cool, I’m not really familiar with clap so I wasn’t aware this was a thing, thank you very much!

Nemo157 · 2020-02-26T10:05:23Z

src/init/starship.bash

@@ -24,7 +24,7 @@ starship_preexec() {
 # Will be run before the prompt is drawn
 starship_precmd() {
    # Save the status, because commands in this pipeline will change $?
-    STATUS=$?
+    local STATUS=$? PIPE="${PIPESTATUS[@]}";


It looks when using bash-preexec this will have to be BP_PIPESTATUS: https://github.com/rcaloras/bash-preexec/blob/master/bash-preexec.sh#L41-L45

tiffafoo · 2020-05-19T22:59:53Z

Closing this in favor of a newer one: #1165

This MR is based on this one starship#370

* feat: Add pipestatus display in status module This MR is based on this one #370 * Documentation * Add a test with map_symbol false * Handle bash preexec pipestatus * Add zsh support * Add fish support Thanks kidonng for the diff patch * Rename sucess_symbol to success_symbol

nrabulinski added 3 commits September 14, 2019 13:24

feat: implement status module

fee6188

test: Unit tests for status module

2730603

fix: clippy failing in status

c09bacd

nrabulinski marked this pull request as ready for review September 14, 2019 14:16

nrabulinski added 2 commits September 14, 2019 17:43

fix: negated pipelines

dba40f6

fix: pipestatus in bash

3bc29ff

jletey reviewed Sep 15, 2019

View reviewed changes

jletey changed the title ~~feat: module for showing exit codes from previous pipeline~~ feat: Add module for showing exit codes of shell pipeline Sep 15, 2019

jletey and others added 2 commits September 15, 2019 12:20

Merge branch 'master' into feat/exit_status

18ab803

refactor: change status module options to an enum

00c94ef

fix: 130 status when ^C is sent to the terminal

2d19dfd

nrabulinski force-pushed the feat/exit_status branch from d7ada94 to 7892563 Compare September 15, 2019 20:01

refactor: negation as a separate segment

778d250

and a few smaller fixes one being changing delimeter_... to prefix/suffix as in starship#367

nrabulinski force-pushed the feat/exit_status branch from 7892563 to 778d250 Compare September 15, 2019 20:05

fix: unnecessary set style

cb08722

nrabulinski added 4 commits September 29, 2019 13:59

feat: status module refactor

e747e7e

test: status tests

b332635

Merge branch 'master' into feat/exit_status

cdecd35

merge master to feat/exit_status

dd8916a

nrabulinski added 3 commits December 10, 2019 20:23

Fixes for merging master

54b7f2f

Written docs for status module

f8c4bea

Merge branch 'master' into feat/exit_status

3d04603

matchai reviewed Dec 24, 2019

View reviewed changes

matchai requested a review from a team December 24, 2019 04:02

jletey added 2 commits January 11, 2020 21:35

Format

cf22482

Attempt to fix Clippy

db651ee

chipbuster mentioned this pull request Jan 26, 2020

Support for status codes #882

Closed

Lucretiel reviewed Jan 27, 2020

View reviewed changes

matchai force-pushed the master branch from e3a30c3 to c4ab66d Compare February 6, 2020 16:46

Nemo157 reviewed Feb 26, 2020

View reviewed changes

Merge branch 'master' into feat/exit_status

01045d3

matchai mentioned this pull request May 7, 2020

feat: add the status module #1165

Closed

8 tasks

tiffafoo closed this May 19, 2020

izissise added a commit to izissise/starship that referenced this pull request Mar 20, 2021

Add pipestatus parameters and format codes

c913047

This MR is based on this one starship#370

izissise mentioned this pull request Mar 20, 2021

feat(status): Add pipestatus display in status module #2481

Merged

8 tasks

izissise added a commit to izissise/starship that referenced this pull request Mar 22, 2021

feat: Add pipestatus display in status module

ea9c22a

This MR is based on this one starship#370

izissise added a commit to izissise/starship that referenced this pull request Mar 26, 2021

feat: Add pipestatus display in status module

0bdf9dc

This MR is based on this one starship#370

izissise added a commit to izissise/starship that referenced this pull request Mar 27, 2021

feat: Add pipestatus display in status module

d789d26

This MR is based on this one starship#370

izissise added a commit to izissise/starship that referenced this pull request Apr 7, 2021

feat: Add pipestatus display in status module

8899df0

This MR is based on this one starship#370

izissise added a commit to izissise/starship that referenced this pull request Apr 17, 2021

feat: Add pipestatus display in status module

53a1c4b

This MR is based on this one starship#370

izissise added a commit to izissise/starship that referenced this pull request May 24, 2021

feat: Add pipestatus display in status module

8299143

This MR is based on this one starship#370

izissise added a commit to izissise/starship that referenced this pull request Jul 1, 2021

feat: Add pipestatus display in status module

c25e534

This MR is based on this one starship#370

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add module for showing exit codes of shell pipeline #370

feat: Add module for showing exit codes of shell pipeline #370

nrabulinski commented Sep 14, 2019 •

edited by andytom

Loading

nrabulinski commented Sep 14, 2019

nickwb commented Sep 15, 2019

jletey left a comment

nrabulinski commented Sep 15, 2019

lilyball commented Sep 16, 2019

nrabulinski commented Sep 16, 2019

lilyball commented Sep 17, 2019

nrabulinski commented Sep 17, 2019

nkoehring commented Sep 17, 2019

lilyball commented Sep 19, 2019

chipbuster commented Sep 25, 2019

nrabulinski commented Sep 26, 2019

matchai left a comment •

edited

Loading

matchai Dec 24, 2019

matchai Dec 24, 2019

matchai Dec 24, 2019

matchai Dec 24, 2019

chipbuster commented Dec 29, 2019

lilyball commented Dec 29, 2019

jletey commented Jan 12, 2020

matchai commented Jan 12, 2020

Lucretiel Jan 27, 2020

Lucretiel Jan 27, 2020 •

edited

Loading

nrabulinski Feb 5, 2020

Nemo157 Feb 26, 2020

tiffafoo commented May 19, 2020

feat: Add module for showing exit codes of shell pipeline #370

feat: Add module for showing exit codes of shell pipeline #370

Conversation

nrabulinski commented Sep 14, 2019 • edited by andytom Loading

Description

Motivation and Context

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

nrabulinski commented Sep 14, 2019

nickwb commented Sep 15, 2019

jletey left a comment

Choose a reason for hiding this comment

nrabulinski commented Sep 15, 2019

lilyball commented Sep 16, 2019

nrabulinski commented Sep 16, 2019

lilyball commented Sep 17, 2019

nrabulinski commented Sep 17, 2019

nkoehring commented Sep 17, 2019

lilyball commented Sep 19, 2019

chipbuster commented Sep 25, 2019

nrabulinski commented Sep 26, 2019

matchai left a comment • edited Loading

Choose a reason for hiding this comment

matchai Dec 24, 2019

Choose a reason for hiding this comment

matchai Dec 24, 2019

Choose a reason for hiding this comment

matchai Dec 24, 2019

Choose a reason for hiding this comment

matchai Dec 24, 2019

Choose a reason for hiding this comment

chipbuster commented Dec 29, 2019

lilyball commented Dec 29, 2019

jletey commented Jan 12, 2020

matchai commented Jan 12, 2020

Lucretiel Jan 27, 2020

Choose a reason for hiding this comment

Lucretiel Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

nrabulinski Feb 5, 2020

Choose a reason for hiding this comment

Nemo157 Feb 26, 2020

Choose a reason for hiding this comment

tiffafoo commented May 19, 2020

nrabulinski commented Sep 14, 2019 •

edited by andytom

Loading

matchai left a comment •

edited

Loading

Lucretiel Jan 27, 2020 •

edited

Loading