Summary

Introduce the api-get command.

Review Request #4195 — Created June 2, 2013 and submitted June 10, 2013, 5:54 p.m.

Information

Owner

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

People

chipx86, mw_triad

Description

Introduce the api-get command.

The new rbt api-get command takes a path to an api resource, requests
the resource, and prints it as JSON. The path can either be rooted
at the api level (e.g. '/review-requests/123/') or be a full url.

Query arguments may also be specified for the request. Each query
argument, appearing after '--', taking the form
'--<query-arg>=<value>' will be added to the request. For example,
'rbt api-get /review-requests/ -- --counts-only=1' would result in
a request to 'http://example.com/api/review-requests/?counts-only=1'.

To support this new command, a new 'get_url' is now required for
transports. This change provides the required method for the
SyncTransport.

Testing Done

Requested a number of resources with different query arguments and
observed the expected JSON output. Also tested some error conditions
such as print the error payloads when requesting non-existent
review-request resources (ids which don't exist).

Issues

Description	From	Last Updated
"URL"	chipx86	June 3, 2013, 9:25 a.m.
"URL". Same with other instances of "url."	chipx86	June 3, 2013, 9:25 a.m.
Comma after ``https://`	chipx86	June 3, 2013, 9:26 a.m.
This is kind of worded strangely. Maybe: "Each query argument appears after ``--`` on the command line and takes the …	chipx86	June 3, 2013, 9:27 a.m.
Maybe define this example as its own indented block.	chipx86	June 3, 2013, 9:32 a.m.
Should be "rbt api-get"	chipx86	June 3, 2013, 9:32 a.m.
This is too specific to this command. Maybe we start doing something like: command_options = { 'api-get': { 'PRETTY_PRINT': True, …	chipx86	June 3, 2013, 9:50 a.m.
"Retrieve" "URL"	chipx86	June 3, 2013, 9:34 a.m.
"URL" Maybe "The URL is not guaranteed to be part of the configured Review Board domain."	chipx86	June 3, 2013, 9:34 a.m.
Should ideally use endswith() for readability.	chipx86	June 3, 2013, 9:37 a.m.
"Retrieve" Maybe "Retrieve raw API resource payloads."	chipx86	June 3, 2013, 9:39 a.m.
"Pretty" should be capitalized, like the others.	chipx86	June 3, 2013, 9:39 a.m.
No need for the if statement. args is guaranteed to be iterable.	chipx86	June 3, 2013, 9:42 a.m.
"retrieve"	chipx86	June 3, 2013, 9:44 a.m.
This is repeated twice. Let's just make a self._dumps function that does this.	chipx86	June 3, 2013, 9:47 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/api/client.py
    rbtools/api/resource.py
    rbtools/api/transport/__init__.py
    rbtools/commands/api_get.py
    rbtools/api/decode.py
    rbtools/api/transport/sync.py
    setup.py
  Ignored Files:
    docs/rbtools/rbt/commands/index.txt
    docs/rbtools/rbt/commands/api-get.txt

docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)
The issue has been resolved. Show all issues
```
"URL"
```
docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)
The issue has been resolved. Show all issues
```
"URL".

Same with other instances of "url."
```
docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)
The issue has been resolved. Show all issues
```
Comma after ``https://`
```

docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)

The issue has been resolved. Show all issues

This is kind of worded strangely. Maybe:
 "Each query argument appears after ``--`` on the command line and takes the form of ``--<query-arg>=<value>``."

I don't think you need the last "will be added to the request," since that's implied by the previous sentence.

docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)
The issue has been resolved. Show all issues
```
Maybe define this example as its own indented block.
```
docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be "rbt api-get"
```

docs/rbtools/rbt/commands/api-get.txt (Diff revision 1)

The issue has been resolved. Show all issues

This is too specific to this command. Maybe we start doing something like:

command_options = {
    'api-get': {
        'PRETTY_PRINT': True,
    }
}

Or if that's too Pythonic or troublesome, maybe just API_GET_PRETTY_PRINT. Not sure.

SM

smacleod June 3, 2013, 9:50 a.m.

I went with API_GET_PRETTY_PRINT.

I'd rather keep things as flat as possible right now. I'm always wanting to move
away from having .reviewboardrc files evaluated as code to deal with configuration
and documenting more complex configuration like this might make things difficult if
we try and make the jump at some point.

rbtools/api/transport/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Retrieve"

"URL"
```

rbtools/api/transport/__init__.py (Diff revision 1)

The issue has been resolved. Show all issues

"URL"

Maybe "The URL is not guaranteed to be part of the configured Review Board domain."

rbtools/api/transport/sync.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should ideally use endswith() for readability.
```
rbtools/commands/api_get.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Retrieve"

Maybe "Retrieve raw API resource payloads."
```
rbtools/commands/api_get.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Pretty" should be capitalized, like the others.
```

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

The issue has been resolved. Show all issues

No need for the if statement. args is guaranteed to be iterable.

SM

smacleod June 3, 2013, 9:50 a.m.

Ah, it was a leftover from when the args were used for a different purpose
where behavior changed if there were any.

rbtools/commands/api_get.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"retrieve"
```
1. SM
  
  smacleod June 3, 2013, 9:50 a.m.
  Wow, apparently I never spell this correctly. Sorry you had to be my spellcheck :/

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

The issue has been resolved. Show all issues

This is repeated twice. Let's just make a self._dumps function that does this.

SM

Change Summary:

Updated based on Christian's review.

Diff:

Revision 2 (+170 -5)

	setup.py
	docs/rbtools/rbt/commands/api-get.txt
	docs/rbtools/rbt/commands/index.txt
	rbtools/api/client.py
	rbtools/api/decode.py
	rbtools/api/resource.py
	rbtools/api/transport/__init__.py
	rbtools/api/transport/sync.py
	1 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/api/client.py
    rbtools/api/resource.py
    rbtools/api/transport/__init__.py
    rbtools/commands/api_get.py
    rbtools/api/decode.py
    rbtools/api/transport/sync.py
    setup.py
  Ignored Files:
    docs/rbtools/rbt/commands/index.txt
    docs/rbtools/rbt/commands/api-get.txt

Ship it!

```
Ship It!
```

SM

Status: Closed (submitted)

Change Summary:

Pushed to master, release-0.5.x (fc01bea128a67c0f3a318a75ad47c425d844d8de).

Loading...