Conversation
|
GNU testsuite comparison: |
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
As you only handle the error case, you can simplify it with map_err:
| match apply_char_mapping(&mut termios, char_index, new_cc) { | |
| Ok(_) => {} | |
| Err(e) => match e { | |
| ControlCharMappingError::IntOutOfRange => { | |
| return Err(USimpleError::new( | |
| 1, | |
| format!( | |
| "invalid integer argument: '{new_cc}': Numerical result out of range" | |
| ), | |
| )); | |
| } | |
| ControlCharMappingError::MultipleChars => { | |
| return Err(USimpleError::new( | |
| 1, | |
| format!("invalid integer argument: '{new_cc}'"), | |
| )); | |
| } | |
| }, | |
| } | |
| apply_char_mapping(&mut termios, char_index, new_cc).map_err(|e| { | |
| let message = match e { | |
| ControlCharMappingError::IntOutOfRange => format!( | |
| "invalid integer argument: '{new_cc}': Numerical result out of range" | |
| ), | |
| ControlCharMappingError::MultipleChars => { | |
| format!("invalid integer argument: '{new_cc}'") | |
| } | |
| }; | |
| USimpleError::new(1, message) | |
| })?; |
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
This function name is a bit misleading: functions starting with is_ usually return a bool.
There was a problem hiding this comment.
initially i was returning a bool but then changed it to an option, good catch
9d0ca57 to
8a6e390
Compare
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
I think you can simplify it to something like:
| match string_to_control_char(new_val) { | |
| Ok(val) => { | |
| termios.control_chars[control_char_index as usize] = val; | |
| Ok(()) | |
| } | |
| Err(e) => Err(e), | |
| } | |
| string_to_control_char(new_val).and_then(|val| { | |
| termios.control_chars[control_char_index as usize] = val; | |
| Ok(()) | |
| }) |
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
The function returns a ControlCharMappingError in the error case, not None.
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
ascii_num doesn't have to be mutable. And you can get rid of the last if let:
| let mut ascii_num: Option<u32> = None; | |
| if let Some(hex) = s.strip_prefix("0x") { | |
| ascii_num = u32::from_str_radix(hex, 16).ok(); | |
| } else if let Some(octal) = s.strip_prefix("0") { | |
| ascii_num = u32::from_str_radix(octal, 8).ok(); | |
| } else if let Ok(decimal) = s.parse::<u32>() { | |
| ascii_num = Some(decimal); | |
| } | |
| let ascii_num = if let Some(hex) = s.strip_prefix("0x") { | |
| u32::from_str_radix(hex, 16).ok() | |
| } else if let Some(octal) = s.strip_prefix("0") { | |
| u32::from_str_radix(octal, 8).ok() | |
| } else { | |
| s.parse::<u32>().ok() | |
| }; |
|
GNU testsuite comparison: |
|
anything else you'd like me to fix? i can push the final review commit now if we're all good |
|
@willshuttleworth from my side it looks good, maybe @tertsdiepraam has something to add as he wrote most of the current implementation. |
|
@cakebaker i have committed the final code review related changes |
|
GNU testsuite comparison: |
23f7c1b to
fdbd248
Compare
|
it would be nice to have tests |
|
the one issue with testing is that |
|
that would be ideal |
550aa8b to
20134d2
Compare
|
ok, i have reworked the earlier changes so that some testing can be done. the new approach is to parse and collect all arguments, return errors if necessary, then apply the new flags. none of the tty related calls are made until parsing is complete, so errors during parsing can be tested for. |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
0f4067e to
e1eee82
Compare
|
GNU testsuite comparison: |
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
You can simplify the match to:
| let mut remove_group = false; | |
| match flag { | |
| AllFlags::Baud(_) => {} | |
| AllFlags::ControlFlags((flag, remove)) => { | |
| remove_group = check_flag_group(flag, remove); | |
| } | |
| AllFlags::InputFlags((flag, remove)) => { | |
| remove_group = check_flag_group(flag, remove); | |
| } | |
| AllFlags::LocalFlags((flag, remove)) => { | |
| remove_group = check_flag_group(flag, remove); | |
| } | |
| AllFlags::OutputFlags((flag, remove)) => { | |
| remove_group = check_flag_group(flag, remove); | |
| } | |
| } | |
| let remove_group = match flag { | |
| AllFlags::Baud(_) => false, | |
| AllFlags::ControlFlags((flag, remove)) => check_flag_group(flag, remove), | |
| AllFlags::InputFlags((flag, remove)) => check_flag_group(flag, remove), | |
| AllFlags::LocalFlags((flag, remove)) => check_flag_group(flag, remove), | |
| AllFlags::OutputFlags((flag, remove)) => check_flag_group(flag, remove), | |
| }; |
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
You can simplify it to:
| if remove && flag.group.is_some() { | |
| return true; | |
| } | |
| false | |
| remove && flag.group.is_some() |
src/uu/stty/src/stty.rs
Outdated
There was a problem hiding this comment.
I think you can simplify this with something like:
let remove = option.starts_with('-');
let name = option.trim_start_matches('-');This assumes that option doesn't start with multiple -. Otherwise you could use strip_prefix with unwrap_or_else.
e1eee82 to
0bd75fa
Compare
|
@cakebaker i have integrated the changes from your review |
|
GNU testsuite comparison: |
…uped now, ie 'stty erase ^H'
0bd75fa to
7ce7b5c
Compare
|
GNU testsuite comparison: |
|
Thanks! |
|
thank you for all the help :) also, we can close this issue now. |
Added functionality to set mappings between control characters (erase, intr, kill, etc.) and specific characters given by the user (^C, for example).
I had to rework the argument processing as it was previously processing one flag at a time, but this feature requires processing of consecutive args (ie
stty erase ^C). Addedstring_to_control_char()to convert strings like "^C", "X", "0x7F" to their ASCII value if they are valid control char mappings. Also addedapply_char_mapping()to set new control char mappings using termios.I'm a relatively new Rust programmer, so I'd love feedback on things that could be written more idiomatically or concisely. This PR is related to issue #7357.