Catch editor exiting with nonzero status

Review Request #6843 — Created Jan. 31, 2015 and discarded

Information

RBTools
master

Reviewers

Now edit_text doesn't try and read the temp file if the editor launches, but
exits with a nonzero status code.

I need a hand here, it looks like edit_text is only called from merge, and I'm not sure how rbt merge works.

This should not ship till I test this.

Description From Last Updated

local variable 'return_code' is assigned to but never used

reviewbotreviewbot

undefined name 'neturn_code'

reviewbotreviewbot

I don't know what exception type is appropriate here. It's either an error with the editor, or the user wanting …

TB tbelaire

"go" -> "use" Period after intentionally. "in the meantime" -> "previously"

brenniebrennie

Should we print this here, or should we leave this up to the caller to print an error message (perhaps …

brenniebrennie

This should be in the past tense (e.g., "User aborted the editor.")

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/console.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/console.py
    
    
  2. rbtools/utils/console.py (Diff revision 1)
     
     
     local variable 'return_code' is assigned to but never used
    
  3. rbtools/utils/console.py (Diff revision 1)
     
     
     undefined name 'neturn_code'
    
  4. 
      
TB
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/console.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/console.py
    
    
  2. 
      
TB
  1. 
      
  2. rbtools/utils/console.py (Diff revisions 1 - 2)
     
     

    I don't know what exception type is appropriate here. It's either an error with the editor, or the user wanting to abort.

    1. I wonder if "RuntimeError" is more appropriate. Or perhaps we can create our own subclass of StandardError for this particular case.

    2. My personal rule of thumb is that you aren't actually re-using it, or catching it anywhere, it's likely overkill to make a new class. Runtime error sounds good to me.

  3. 
      
TB
brennie
  1. 
      
  2. rbtools/utils/console.py (Diff revision 2)
     
     

    "go" -> "use"

    Period after intentionally.

    "in the meantime" -> "previously"

  3. rbtools/utils/console.py (Diff revision 2)
     
     

    Should we print this here, or should we leave this up to the caller to print an error message (perhaps with more context)?

  4. rbtools/utils/console.py (Diff revision 2)
     
     

    This should be in the past tense (e.g., "User aborted the editor.")

  5. 
      
brennie
  1. Ping?

  2. 
      
TB
Review request changed

Status: Discarded

Loading...