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

fix(status): Enable to convert from i64 to hex_status by casting instead of parsing status. #3462

Merged
merged 3 commits into from Feb 5, 2022

Conversation

moko256
Copy link
Contributor

@moko256 moko256 commented Jan 19, 2022

This PR will enable to convert from i64 to hex_status by casting instead of parsing status.

Description

In elvish shell on Windows, the status code is passed as u32, and it can't parse as i32, so hex_status is same as status.
This PR will replace parsing status to i32 by casting from i64 to i32 and accept this.
The status code out of i32 will be rounded by underflow/overflow.

Motivation and Context

Closes #
Related #3459

Screenshots (if appropriate):

In powershell In elvish before fix In elvish after fix
A screenshot in powershell A screenshot in elvish before fix A screenshot in elvish after fix
[status]
disabled = false
format = "[$symbol$status $hex_status]($style) "

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.

.parse::<i32>()
.ok()
.map(|code| format!("0x{:X}", code));
let hex_status = format!("0x{:X}", exit_code_int as u32);
Copy link
Member

Choose a reason for hiding this comment

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

I think this will just work and avoid truncation:

Suggested change
let hex_status = format!("0x{:X}", exit_code_int as u32);
let hex_status = format!("0x{:X}", exit_code_int);

Copy link
Contributor Author

@moko256 moko256 Jan 19, 2022

Choose a reason for hiding this comment

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

Avoiding truncation makes hex_status longer when status code is negative number like this:

✖-1 0xFFFFFFFFFFFFFFFF ❯
✖-1073741510 0xFFFFFFFFC000013A ❯

On Windows, status code is in range of u32 or i32 (e.g, MSVC exit() receive int), so I think this cast is necessary to make easy to see.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, should this also apply to int too? Furthermore, maybe this should be restricted to Windows (with another type ExitCodeTarget?) and I feel like this should be documented in the status docs.

Copy link
Contributor Author

@moko256 moko256 Jan 23, 2022

Choose a reason for hiding this comment

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

Whether i32 or u32 the status fits in is consistent in one shell, and in supported platform the status code fits in 32bit.
Until the platform that have larger status code than 32bit will be supported, I think that assuming that the status code fits in 32bit is no problem and hex_status doesn't have to be restricted only for Windows. Similally, I think it doesn't have to apply to int too.
But I think that the size of status code that the module receives should be documented.

Copy link
Contributor Author

@moko256 moko256 Jan 23, 2022

Choose a reason for hiding this comment

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

Instead of casting here, it is also possible to use i32 as ExitCode like this:

diff --git a/src/modules/status.rs b/src/modules/status.rs
index 5f643a95..31d26c2c 100644
--- a/src/modules/status.rs
+++ b/src/modules/status.rs
@@ -6,7 +6,7 @@ use crate::configs::status::StatusConfig;
 use crate::formatter::{string_formatter::StringFormatterError, StringFormatter};
 use crate::segment::Segment;
 
-type ExitCode = i64;
+type ExitCode = i32;
 type SignalNumber = u32;
 #[derive(PartialEq)]
 enum PipeStatusStatus<'a> {
@@ -101,15 +101,16 @@ fn format_exit_code<'a>(
     config: &'a StatusConfig,
     context: &'a Context,
 ) -> Result<Vec<Segment>, StringFormatterError> {
-    let exit_code_int: ExitCode = match exit_code.parse() {
-        Ok(i) => i,
+    // First, parse as i64 to accept both i32 or u32, then normalize to i32.
+    let exit_code_int: ExitCode = match exit_code.parse::<i64>() {
+        Ok(i) => i as ExitCode,
         Err(_) => {
             log::warn!("Error parsing exit_code string to int");
             return Ok(Vec::new());
         }
     };
 
-    let hex_status = format!("0x{:X}", exit_code_int as u32);
+    let hex_status = format!("0x{:X}", exit_code_int);
 
     let common_meaning = status_common_meaning(exit_code_int);
 

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I think it's better to leave int as-is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I pushed the diff to this PR. Thanks for reviewing!

Copy link
Member

@davidkna davidkna Jan 27, 2022

Choose a reason for hiding this comment

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

Please also add a note in the status module description in docs/config/README.md about this and that hex_status is unsigned. (sorry, looks like you ended up removing that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/context.rs Outdated Show resolved Hide resolved
moko256 and others added 2 commits January 28, 2022 02:16
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
@davidkna davidkna merged commit bbdb584 into starship:master Feb 5, 2022
@davidkna
Copy link
Member

davidkna commented Feb 5, 2022

Thanks for handling this @moko256!

Perniciosius pushed a commit to Perniciosius/starship that referenced this pull request Feb 21, 2022
…ead of parsing status. (starship#3462)

* fix(status): Enable to convert from i64 to hex_status by casting instead of parsing status.

* Apply comment to src/context.rs

Co-authored-by: David Knaack <davidkna@users.noreply.github.com>

* Update README.md in configuration

Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
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.

None yet

2 participants