Output a better error message when running rbt patch --print

Review Request #9490 — Created Jan. 16, 2018 and updated

jshephard
RBTools
release-0.7.x
4628
a38267b...
rbtools, students

When running rbt patch --print outside of a repository directory and
without providing the --server parameter or having a ~/.reviewboardrc
file containing REVIEWBOARD_URL, the command would output that the
current directory does not contain a supported repository.

The command is expected to work outside these repository directories. By
improving the error handling in rbtools/clients/__init__.py and in
rbtools/commands/patch.py we can output a more precise error message.

Tested rbt patch --print both inside and outside of directories with
.reviewboardrc files, as well as using --server. Manually verified
other functions calling initialize_scm_tool will continue to function
as is and bubble up the new exceptions being raised.

Description From Last Updated

The description will be a bit easier to read with backticks around things like rbt patch --show, .reviewboardrc, REVIEWBOARD_URL, etc. ...

chipx86chipx86

The testing information should wrap at around the same length as the description.

chipx86chipx86

Nit: in general, we like to put lists of things like this, imports, etc, in alphabetical order. So this should ...

mike_conleymike_conley

Can you move the string to the next line? That will be a little nicer, since it can be self-contained. ...

chipx86chipx86

Mind changing "url" to "URL" while here?

chipx86chipx86

The newline should be removed. Can you also say "for this type of repository" instead of "current SCM client."? I ...

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

Since the semantics of these errors have changed (they're not raised instead of a clean log + exit), all callers ...

chipx86chipx86

This is discarding the potentially very helpful information in the original exception.

chipx86chipx86
mike_conley
  1. This looks fantastic to me - just one minor cosmetic issue. The rest looks good to me.

  2. rbtools/commands/patch.py (Diff revision 1)
     
     
     

    Nit: in general, we like to put lists of things like this, imports, etc, in alphabetical order. So this should probably go before line 4.

    1. Oops! I was aware of this, but forgot here. Thanks!

    2. Make sure you mark issues that you've fixed as "Fixed".

  3. 
      
jshephard
chipx86
  1. 
      
  2. The description will be a bit easier to read with backticks around things like rbt patch --show, .reviewboardrc, REVIEWBOARD_URL, etc. as that will help visually distinguish those as self-contained literals/files/etc.

  3. The testing information should wrap at around the same length as the description.

  4. rbtools/clients/__init__.py (Diff revision 2)
     
     
     

    Can you move the string to the next line? That will be a little nicer, since it can be self-contained. The % client_name would then move to the following, like:

    raise OptionsCheckError(
        '...'
        % client_name)
    
  5. rbtools/clients/__init__.py (Diff revision 2)
     
     

    Mind changing "url" to "URL" while here?

  6. rbtools/clients/__init__.py (Diff revision 2)
     
     
     

    The newline should be removed.

    Can you also say "for this type of repository" instead of "current SCM client."? I know it's not your text originally, but while here, we might as well remove the confusing internal-ish text.

  7. rbtools/clients/__init__.py (Diff revision 2)
     
     

    And here.

  8. rbtools/clients/__init__.py (Diff revision 2)
     
     
     

    And here.

  9. rbtools/commands/patch.py (Diff revision 2)
     
     
     
     
     

    Since the semantics of these errors have changed (they're not raised instead of a clean log + exit), all callers of initialize_scm_tool will need to also catch/re-raise the exception.

    1. Do we want to handle them in the same way patch.py handles it? i.e. outputting the same error message alongside the original message

    2. I'm not seeing patch.py handling this. I might not be understanding what you're referring to (sleepy).

    3. Sorry, maybe I misunderstood your question. If all callers to initialize_scm_tool will handle the exception in the same way (i.e. re-raising), I think it would make sense to handle it in initialize_scm_tool instead. That function currently handles OptionsCheckError by raising it as a CommandError (commands\__init__.py line 698).

  10. rbtools/commands/patch.py (Diff revision 2)
     
     
     
     
     
     

    This is discarding the potentially very helpful information in the original exception.

    1. So in this situation, what would be the best approach?

      I could print out the message ('Could not determine [...]') then re-raise the given NoRepositoryError, so the user will see the original exception message.

      Could not determine the Review Board server to connect to. Please specify the server with --server or set REVIEWBOARD_URL in .reviewboardrc.
      CRITICAL: The current directory does not contain a checkout from a supported source code repository.
      

      Or I could instead print NoRepositoryError's message then raise the CommandError exception.

      The current directory does not contain a checkout from a supported source code repository.
      ERROR: Could not determine the Review Board server to connect to. Please specify the server with --server or set REVIEWBOARD_URL in .reviewboardrc.
      
    2. Ideally, users should never see an exception. Errors, sure, but an exception with a backtrace is a crash, and we want to avoid that. The commands should raise a CommandError, but we should strive to have the original error messages be sufficient. If they can properly convey what needs to be done, no need to have a custom error in CommandError.

      You can pass the NoRepositoryError's message in CommandError with:

      raise CommandError(six.text_type(e))
      
    3. How does this exception message sound: The current directory does not contain a checkout from a supported source code repository. Please specify the Review Board server with --server or set REVIEWBOARD_URL in .reviewboardrc.

  11. 
      
jshephard
jshephard
Review request changed

Change Summary:

Adjusted the exception messages as per Christian's review.

Summary:

-Output a better error message when running rbt patch --show
+Output a better error message when running rbt patch --print

Description:

~  

When running rbt patch --show outside of a repository directory and

  ~

When running rbt patch --print outside of a repository directory and

    without providing the --server parameter or having a ~/.reviewboardrc
    file containing REVIEWBOARD_URL, the command would output that the
    current directory does not contain a supported repository.

   
   

The command is expected to work outside these repository directories. By

    improving the error handling in rbtools/clients/__init__.py and in
    rbtools/commands/patch.py we can output a more precise error message.

Testing Done:

~  

Tested rbt patch --show both inside and outside of directories with

  ~

Tested rbt patch --print both inside and outside of directories with

    .reviewboardrc files, as well as using --server. Manually verified
    other functions calling initialize_scm_tool will continue to function
    as is and bubble up the new exceptions being raised.

Commit:

-ee6b86b18ba2187d9e7217dcab2a000143e1a869
+a38267bd20014927470302c3cafa92cd373b6139

Diff:

Revision 3 (+31 -23)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...