Summary

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

Review Request #9490 — Created Jan. 16, 2018 and discarded Oct. 26, 2021, 1:49 p.m.

Information

Owner

jshephard

Repository

RBTools

Branch

release-0.7.x

Bugs

4628

Depends On

Reviewers

Groups

rbtools, students

People

Description

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 --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.

Issues

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. …	chipx86	Jan. 19, 2018, 11:56 a.m.
The testing information should wrap at around the same length as the description.	chipx86	Jan. 19, 2018, 11:56 a.m.
Nit: in general, we like to put lists of things like this, imports, etc, in alphabetical order. So this should …	mike_conley	Jan. 19, 2018, 8:42 a.m.
Can you move the string to the next line? That will be a little nicer, since it can be self-contained. …	chipx86	Jan. 19, 2018, 11:57 a.m.
Mind changing "url" to "URL" while here?	chipx86	Jan. 19, 2018, 11:57 a.m.
The newline should be removed. Can you also say "for this type of repository" instead of "current SCM client."? I …	chipx86	Jan. 19, 2018, 11:57 a.m.
And here.	chipx86	Jan. 19, 2018, 11:57 a.m.
And here.	chipx86	Jan. 19, 2018, 11:57 a.m.
Since the semantics of these errors have changed (they're not raised instead of a clean log + exit), all callers …	chipx86	Jan. 19, 2018, 11:40 a.m.
This is discarding the potentially very helpful information in the original exception.	chipx86	Jan. 19, 2018, 11:40 a.m.

flake8 passed.

JSHint passed.

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

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

The issue has been resolved. Show all issues

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.

jshephard Jan. 19, 2018, 8:37 a.m.

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

brennie Jan. 19, 2018, 8:41 a.m.

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

Change Summary:

Fix ordering of imports

Commit:

-	0059a42b1e41dbed1328de807c0bc9845c09b3a9
+	ee6b86b18ba2187d9e7217dcab2a000143e1a869

Diff:

Revision 2 (+30 -23)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/commands/patch.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

The issue has been resolved. Show all issues

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.

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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)

rbtools/clients/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Mind changing "url" to "URL" while here?
```

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

The issue has been resolved. Show all issues

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.

rbtools/clients/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
And here.
```
rbtools/clients/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
And here.
```

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

An issue was opened. Show all issues

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.

jshephard Jan. 19, 2018, 12:09 p.m.

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

chipx86 Jan. 19, 2018, 12:20 p.m.

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

jshephard Jan. 19, 2018, 1:20 p.m.

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).

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

An issue was opened. Show all issues

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

jshephard Jan. 19, 2018, 12:09 p.m.

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.

chipx86 Jan. 19, 2018, 12:20 p.m.

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))

jshephard Jan. 19, 2018, 1:20 p.m.

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.

Description:

~		When running rbt patch --show 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
	~	When running `rbt patch --show` 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.
	~	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 .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.
	~	Tested `rbt patch --show` 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.

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

	rbtools/clients/__init__.py
	rbtools/clients/errors.py
	rbtools/commands/patch.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Review Board 7.0 alpha 0 (dev)

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

Information

Reviewers

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Change Summary:

Summary:

Description:

Testing Done:

Commit:

Diff:

Checks run (2 succeeded)

Status: Discarded