- 
                Notifications
    
You must be signed in to change notification settings  - Fork 147
 
feat: add --interactive option to prompt for each change #1119
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
base: master
Are you sure you want to change the base?
Conversation
c83b305    to
    ffd6238      
    Compare
  
    | Ok(()) | ||
| } | ||
| 
               | 
          ||
| fn select_fix(typo: &typos::Typo<'_>) -> Option<usize> { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can reproduce the issue with the cursor (but thought something in my setup is broken, apparently it's not my setup), I've really no idea what causes it. I've tried the inquire library as well because I thought something is wrong with the dialoguer library but that one worked even worse...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well, apparently we have to handle Ctrl+C issues ourselves as per console-rs/dialoguer#294.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the available cli libraries for prompts are not really that mature yet.
5c2fec3    to
    b0bdd7e      
    Compare
  
    b0bdd7e    to
    13b0a28      
    Compare
  
    | 
           Thanks for your review so far, I'm way more comfortable with the code now than before the review.  | 
    
b4cabe8    to
    5fc0015      
    Compare
  
    5fc0015    to
    7f632ac      
    Compare
  
    | 
           I've added a custom  I've no idea why the position of the prompt is weird sometimes.  | 
    
        
          
                crates/typos-cli/src/file.rs
              
                Outdated
          
        
      | .default(true) | ||
| .show_default(true) | ||
| .interact(); | ||
| return confirmation.map(|_| 0).ok(); | 
Check failure
Code scanning / clippy
unneeded `return` statement
        
          
                crates/typos-cli/src/file.rs
              
                Outdated
          
        
      | return match selection { | ||
| Ok(selection) => { | ||
| if selection != 0 { | ||
| Some(selection - 1) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| Err(_) => None, | ||
| }; | 
Check failure
Code scanning / clippy
unneeded `return` statement
| 
               | 
          ||
| // https://github.com/console-rs/dialoguer/issues/294 | ||
| ctrlc::set_handler(move || { | ||
| let _ = console::Term::stdout().show_cursor(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now see the cursor but I can't see anything I type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's strange, for me everything behaves normal after that change.
| // https://github.com/console-rs/dialoguer/issues/294 | ||
| ctrlc::set_handler(move || { | ||
| let _ = console::Term::stdout().show_cursor(); | ||
| std::process::exit(0); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is exit(0) the correct way of exiting on ctrl-c to mirror behavior before this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, the previous exit code for ctrl-c was 130, and according to my quick research 130 is preferred over 0 when ctrl-c killed the process via SIGTERM.
          
 For me, its always weird for the first prompt. This would be a blocker for merging  | 
    
          
 I see, but unfortunately I don't see any way to fix this except for trying other libraries?  | 
    
| 
           How about we switch to showing prompts that require hitting   | 
    
          
 That could fix the issue for the  If I do however run this on different source code folders, for some everything works normally, and for some it behaves as described above, which doesn't make a lot of sense to me. The only two considerable crates I found are the   | 
    
7f632ac    to
    a843d91      
    Compare
  
    a843d91    to
    b435635      
    Compare
  
    

The code is probably far from perfect, I'm hoping for some help at improving the code quality in the review :)
closes #397