Summary

Replaced multiple get_review_request methods with an equivalent utility function

Review Request #6348 — Created Sept. 20, 2014 and submitted Sept. 21, 2014, 6:58 a.m.

Information

Owner

asalahli

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

8f14536...

Reviewers

Groups

rbtools, students

People

Description

There was a method called get_review_request() in 4 different classes with identical

bodies. They have now been replaced with a utility function in rbtools.utils.commands.

Testing Done

Manually tested attach, close, post, publish commands.

It seems difficult to add new unit tests to verify the correctness of this new
change. And the existing unit tests pass even if the commands are fatally broken.

Issues

Description	From	Last Updated
'APIError' imported but unused	reviewbot	Sept. 20, 2014, noon
'Option' imported but unused	reviewbot	Sept. 20, 2014, noon
Blank line between this and the if statement.	chipx86	Sept. 20, 2014, 12:14 p.m.
Can you add a blank line after this?	chipx86	Sept. 20, 2014, 12:14 p.m.
There still is another version of this function in rbtools.hooks.common, but they differ in the exception they are raising. One …	AS asalahli	Sept. 21, 2014, 6:45 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Sept. 20, 2014, 12:21 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Sept. 20, 2014, 12:21 p.m.

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py

rbtools/commands/close.py (Diff revision 1)
Show all issues
```
 'APIError' imported but unused
```
rbtools/commands/publish.py (Diff revision 1)
Show all issues
```
 'Option' imported but unused
```

Commit:

-	bfdb68155894083e7979057bb551f81ee3ca9d7c
+	9e3cdc34ca3834b571f9598662bc11a61298592a

Diff:

Revision 2 (+23 -40)

Show changes

	rbtools/commands/attach.py
	rbtools/commands/close.py
	rbtools/commands/post.py
	rbtools/commands/publish.py
	rbtools/utils/review.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py

rbtools/utils/review.py (Diff revision 2)

Show all issues

There still is another version of this function in rbtools.hooks.common, but they differ in the exception they are raising. One raises CommandError and other does HookError. Should I leave them that way?

chipx86 Sept. 20, 2014, 12:06 p.m.

Might be nice to have a ReviewRequestNotFound error instead, and then the other callers can catch and raise their own respective error based on the text contents of that one.

chipx86 Sept. 20, 2014, 12:09 p.m.

Actually, then we're really doing nothing better than each function having their own get_review_request... Let me think on this.

asalahli Sept. 20, 2014, 12:10 p.m.

But then every caller will do:
try:
    review_request = get_review_request(request_id, api_root):
except ReviewRequestNotFound, e:
    raise SpecificError("Error getting review request %s: %s"
                        % (review_request_id, e))

which is basically same with the get_review_request function in the first place

asalahli Sept. 20, 2014, 12:28 p.m.

However, if we make ReviewRequestNotFound inherit from both CommandError and HookError, then catching either of those exceptions will also catch ReviewRequestNotFound

chipx86 Sept. 20, 2014, 1:07 p.m.

That's true, but it doesn't really make sense to say that a "review request not found" is a type of a "hook" or "command" failure. It's really its own thing.

I was having some talks with Steven about the organization of rbtools/utils/, which I'm afraid is getting a bit multi-purpose. For now, however, let's go ahead and make a rbtools/utils/commands.py and put it there. It can be a place for utility commands. We'll just leave the hooks usage alone.

I think we'll want to do other work for cleaning up utils, but that's probably a larger task requiring more thought than we should give right now.

rbtools/commands/post.py (Diff revision 2)
Show all issues
```
Blank line between this and the if statement.
```
rbtools/commands/publish.py (Diff revision 2)
Show all issues
```
Can you add a blank line after this?
```

Commit:

-	9e3cdc34ca3834b571f9598662bc11a61298592a
+	d6c6f7ca8f8bd40993ef86077bae8b37874e4a02

Diff:

Revision 3 (+25 -40)

Show changes

	rbtools/commands/attach.py
	rbtools/commands/close.py
	rbtools/commands/post.py
	rbtools/commands/publish.py
	rbtools/utils/review.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py

rbtools/commands/post.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
rbtools/commands/publish.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Commit:

-	d6c6f7ca8f8bd40993ef86077bae8b37874e4a02
+	53c37705ef1cfbfca8bc4159e5be5ffe96ed2d49

Diff:

Revision 4 (+24 -39)

Show changes

	rbtools/commands/attach.py
	rbtools/commands/close.py
	rbtools/commands/post.py
	rbtools/commands/publish.py
	rbtools/utils/review.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/review.py
    rbtools/commands/publish.py
    rbtools/commands/close.py

Commit:

-	53c37705ef1cfbfca8bc4159e5be5ffe96ed2d49
+	8f14536f074dcafe5ef5733c9285ae3748bd1d3c

Diff:

Revision 5 (+24 -39)

Show changes

	rbtools/commands/attach.py
	rbtools/commands/close.py
	rbtools/commands/post.py
	rbtools/commands/publish.py
	rbtools/utils/commands.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/commands.py
    rbtools/commands/publish.py
    rbtools/commands/close.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/utils/commands.py
    rbtools/commands/publish.py
    rbtools/commands/close.py

Ship it!

```
Ship It!
```

Description:

		There was a method called `get_review_request()` in 4 different classes with identical
~		bodies. They have now been replaced with a utility function in rbtools.utils.review.
	~	bodies. They have now been replaced with a utility function in `rbtools.utils.commands`.

Status: Closed (submitted)

Change Summary:

Pushed to release-0.6.x (845188b)