flake8
-
rbtools/api/transport/sync.py (Diff revision 1) Show all issues -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Review Request #11479 — Created Feb. 21, 2021 and updated
Information | |
---|---|
qianxi | |
RBTools | |
master | |
Reviewers | |
rbtools | |
The reason why we need api command instead of api-get is simple: We need to do HTTP GET, POST, PUT, DELETE requests.
I createdapi
command, with new options--get
,--post
,--put
,--delete
,--patch
,--file="local path"
,--data='key1=value1'
,--header='headercontent'
, original--pretty
option and server options still work.
For api
--get
,--post
,--put
,--delete
options, I did some testings using command line tools.I created a review request draft and used my
--get
,--post
,--put
and--delete
options, they all worked fine.
I posted several files to the review request draft using--post
and--file
options and they worked.
I don't think our reviewboard APIs support--patch
at this moment so I just left it there in case one day it will be useful.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
Please remove the .DS_Store file from your change. |
|
|
E225 missing whitespace around operator |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
F401 'rbtools.commands.OptionGroup' imported but unused |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
W503 line break before binary operator |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
W503 line break before binary operator |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E231 missing whitespace after ':' |
![]() |
|
W291 trailing whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W391 blank line at end of file |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W391 blank line at end of file |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W292 no newline at end of file |
![]() |
|
F841 local variable 'e' is assigned to but never used |
![]() |
|
E231 missing whitespace after ',' |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E265 block comment should start with '# ' |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E228 missing whitespace around modulo operator |
![]() |
|
F841 local variable 'e' is assigned to but never used |
![]() |
|
E265 block comment should start with '# ' |
![]() |
|
W291 trailing whitespace |
![]() |
|
E231 missing whitespace after ':' |
![]() |
|
E231 missing whitespace after ':' |
![]() |
|
E231 missing whitespace after ':' |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
F841 local variable 'name' is assigned to but never used |
![]() |
|
This should be reverted. |
|
|
Please restore this blank line. |
|
|
This needs a method docstring. |
|
|
This should probably be defined as a raw string (prefix with r, like `r'(...)+') |
|
|
Should this be KeyError instead of just Exception? |
|
|
Should this be using the data variable you just defined? |
|
|
This needs a method docstring. |
|
|
Please use single quotes instead of double. |
|
|
This needs a method docstring. |
|
|
Blank line between these two |
|
|
This needs a method docstring. |
|
|
This needs a method docstring. |
|
|
Should this be KeyError? |
|
|
We're doing this a lot. Perhaps we can have a helper method to sanitize the URL? |
|
|
Please use single quotes instead of double. |
|
|
This needs a method docstring. |
|
|
This needs a method docstring. |
|
|
Please use single quotes instead of double. |
|
|
This needs a method docstring. |
|
|
This needs a method docstring. |
|
|
Might be nice to have a helper for this too. |
|
|
Please use single quotes instead of double. |
|
|
This needs a method docstring. |
|
|
KeyError? |
|
|
Please use single quotes instead of double. |
|
|
This needs a method docstring. |
|
|
This needs a method docstring. |
|
|
Please revert these changes--it's just churn in blank lines for no purpose. |
|
|
Same here--move the blank line back, please. |
|
|
Please revert the changes in this file. |
|
|
This needs a module docstring. |
|
|
This needs a class docstring. |
|
|
How about "Perform Review Board API requests"? Note also that when we wrap a string, we put the space at … |
|
|
Put the space at the end of the first line instead of beginning of the second. |
|
|
Please swap the use of single and double quotes here. |
|
|
Swap single and double quotes here. |
|
|
This needs a method docstring. |
|
|
This first part needs to fit on a single line. Simply "Handle HTTP requests" is probably sufficient--the arguments are listed … |
|
|
This should probably mention that it prints output. Alternatively, this method could just return the request and the caller could … |
|
|
There's an extra blank line here. |
|
|
This blank line can be removed. |
|
|
Please use single quotes. |
|
|
Please use single quotes. |
|
|
Please use single quotes. |
|
|
Please use single quotes. This should also wrap with each entry on its own line. We could even avoid defining … |
|
|
This needs a docstring. |
|
|
Add a blank line between these two. |
|
|
This should probably print an error (probably different errors depending on if no method was passed, or if multiple methods … |
|
|
This should be reverted. |
|
|
Please revert the changes to this file. |
|
|
Please revert the changes to this file. |
|
|
E501 line too long (80 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
This isn't ultimately useful to have here in our codebase. It's a great thing to learn! But these imports are … |
|
|
Can you rename this to _get_padded_url? That's a better fit for the naming we aim for. We want to communicate … |
|
|
Likewise, this would be better as "Returns a URL ending with a slash." ("URL" should always be capitalized.) |
|
|
For "Returns", the description text can't be indented. We do indent for "Args," because we're have a list of arguments. … |
|
|
"The URL or path ending with a '/'." would be better. |
|
|
We can probably simplify this: if not url.ensdwith('/'): url = '%s/' % url return url Avoids the creation of a … |
|
|
You can probably replace this with Python's urljoin function. |
|
|
No indentation for descriptions in "Returns". This will apply to any others throughout this change. |
|
|
Is this for querystrings? If so, check out Python's parse_qsl function. Look at HttpRequest in RBTools for code already doing … |
|
|
The naming may seem unrelated, but this is actually already available to us in Python's email module as email.message_from_string. This … |
|
|
Should always be capitalized as "URL". Same elsewhere in the change. |
|
|
Repeating ourselves for each method can end up increasing our maintenance burden. Instead, since these are all wrapping a centralized … |
|
|
Instead of dict.items(), we need to do six.iteritems(dict) to get consistent behavior across Python 2 and 3. Same anywhere else … |
|
|
All these _path versions of these functions seem to have the same logic as the _url versions, but are creating … |
|
|
All these blank lines need to remain. |
|
|
This should be removed, for the same reason above. |
|
|
rbt api --post Other examples are also missing the api bit. |
|
|
I think we shouldn't say too much more than "Send a HTTP PATCH request to an API" (and same for … |
|
|
All private functions should follow public functions. |
|
|
You probably want a blank line here. |
|
|
self.options.pretty_print is a reference to internal code. Write these docs like nobody's reading your code. You probably want to say … |
|
|
This is missing a return type. We're also not streaming to anything. We're just returning a string. |
|
|
Comma should be on the prior line. |
|
|
"JSON" |
|
|
You want a blank line before the statement and the loop, but not on the first inner line of the … |
|
|
This can be simplified massively by doing a couple things: Have each HTTP method option (--get, --post, etc.) have an … |
|
|
startswith takes a tuple, for convenience: if path.startswith(('http://', 'https://')): |
|
|
This will need to be the full import class path. |
|
|
I think we want to avoid allowing anything unexpected to be a query argument. What if an API takes a … |
|
|
The above recommendation about the HTTP method options will eliminate all of this. Now, that doesn't mean that it'll duplicate … |
|
|
F401 'email' imported but unused |
![]() |
|
F401 're' imported but unused |
![]() |
|
F401 'urllib.parse.urljoin' imported but unused |
![]() |
|
W291 trailing whitespace |
![]() |
|
F841 local variable 'body_dict' is assigned to but never used |
![]() |
|
F841 local variable 'file_path_list' is assigned to but never used |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
F841 local variable 'file_path_list' is assigned to but never used |
![]() |
|
F841 local variable 'file_path_list' is assigned to but never used |
![]() |
|
F841 local variable 'body_dict' is assigned to but never used |
![]() |
|
F841 local variable 'file_path_list' is assigned to but never used |
![]() |
|
F401 'argparse' imported but unused |
![]() |
|
F401 'rbtools.commands.ParseError' imported but unused |
![]() |
|
F841 local variable 'query_dict' is assigned to but never used |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
F841 local variable 'query_arguments' is assigned to but never used |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
rbtools/api/transport/sync.py (Diff revision 1) |
---|
Fix coding style problems
Commits: |
|
||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+1120 -218) |
Fix more coding style problems.
Commits: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+1124 -226) |
Add a newline character
Commits: |
|
||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+1125 -227) |
Revert commits from another project.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+1385 -617) |
rbtools/api/transport/sync.py (Diff revision 5) |
---|
F841 local variable 'e' is assigned to but never used
Fix coding style problems.
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+1389 -625) |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+1570 -632) |
rbtools/api/transport/sync.py (Diff revision 7) |
---|
F841 local variable 'e' is assigned to but never used
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+1575 -637) |
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+1593 -647) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+1591 -649) |
rbtools/api/transport/sync.py (Diff revision 10) |
---|
This should probably be defined as a raw string (prefix with r, like `r'(...)+')
rbtools/api/transport/sync.py (Diff revision 10) |
---|
Should this be using the
data
variable you just defined?
rbtools/api/transport/sync.py (Diff revision 10) |
---|
We're doing this a lot. Perhaps we can have a helper method to sanitize the URL?
rbtools/api/transport/sync.py (Diff revision 10) |
---|
Please revert these changes--it's just churn in blank lines for no purpose.
rbtools/commands/api.py (Diff revision 10) |
---|
How about "Perform Review Board API requests"?
Note also that when we wrap a string, we put the space at the end of the first line instead of the beginning of the second. Your way is probably less error-prone but all of existing code that does it the other way.
rbtools/commands/api.py (Diff revision 10) |
---|
Put the space at the end of the first line instead of beginning of the second.
rbtools/commands/api.py (Diff revision 10) |
---|
This first part needs to fit on a single line. Simply "Handle HTTP requests" is probably sufficient--the arguments are listed in the method definition and the docstring below.
rbtools/commands/api.py (Diff revision 10) |
---|
This should probably mention that it prints output.
Alternatively, this method could just return the request and the caller could print.
rbtools/commands/api.py (Diff revision 10) |
---|
Please use single quotes. This should also wrap with each entry on its own line. We could even avoid defining the variable and just put it inline:
full_file_list.append({ 'filename': file_name, 'content': content, })
rbtools/commands/api.py (Diff revision 10) |
---|
This should probably print an error (probably different errors depending on if no method was passed, or if multiple methods were passed)
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+2649 -759) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+2653 -763) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+2653 -763) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+2681 -763) |
rbtools/api/transport/sync.py (Diff revision 14) |
---|
This isn't ultimately useful to have here in our codebase. It's a great thing to learn! But these imports are present in every single file in all of our codebases, so we don't want to document it in each file.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
Can you rename this to
_get_padded_url
? That's a better fit for the naming we aim for. We want to communicate that this gets (returns) something, and that the thing it's returning is a padded URL.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
Likewise, this would be better as "Returns a URL ending with a slash."
("URL" should always be capitalized.)
rbtools/api/transport/sync.py (Diff revision 14) |
---|
For "Returns", the description text can't be indented. We do indent for "Args," because we're have a list of arguments. "Returns" doesn't work this way, and this will ultimately render incorrectly.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
"The URL or path ending with a '/'." would be better.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
We can probably simplify this:
if not url.ensdwith('/'): url = '%s/' % url return url
Avoids the creation of a new variable.
%
-based formatting is also generally faster (Python compiles it down to bytecode)
rbtools/api/transport/sync.py (Diff revision 14) |
---|
You can probably replace this with Python's
urljoin
function.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
No indentation for descriptions in "Returns".
This will apply to any others throughout this change.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
Is this for querystrings? If so, check out Python's
parse_qsl
function.Look at
HttpRequest
in RBTools for code already doing this.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
The naming may seem unrelated, but this is actually already available to us in Python's
email.message_from_string
. This is used under-the-hood for Python's own HTTP header parsing, as it's the same standard.You can do something like this (I'm copying from the Python shell):
>>> import email >>> dict(email.message_from_string( ... 'User-Agent: foo\n' ... 'Content-Type: bar\n' ... 'X-Custom: foobar!') {'X-Custom': 'foobar!', 'Content-Type': 'bar', 'User-Agent': 'foo'}
rbtools/api/transport/sync.py (Diff revision 14) |
---|
Should always be capitalized as "URL".
Same elsewhere in the change.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
Repeating ourselves for each method can end up increasing our maintenance burden. Instead, since these are all wrapping a centralized function, it's probably best to just document it there and instead refer readers to that in
args
.However, it's not quite right to pass in a dictionary as an argument and have that contain what's effectively keyword arguments. It would be better to make each of these things a new keyword argument.
Same goes for all other functions doing this.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
Instead of
dict.items()
, we need to dosix.iteritems(dict)
to get consistent behavior across Python 2 and 3.Same anywhere else this may be done.
rbtools/api/transport/sync.py (Diff revision 14) |
---|
All these
_path
versions of these functions seem to have the same logic as the_url
versions, but are creating an absolute URL.How about just keeping the
_path
versions simple. have them call the_url
version, but with the absolute URL (throughurljoin
).
rbtools/commands/api.py (Diff revision 14) |
---|
rbt api --post
Other examples are also missing the
api
bit.
rbtools/commands/api.py (Diff revision 14) |
---|
I think we shouldn't say too much more than "Send a HTTP PATCH request to an API" (and same for other HTTP methods).
rbtools/commands/api.py (Diff revision 14) |
---|
self.options.pretty_print
is a reference to internal code. Write these docs like nobody's reading your code. You probably want to say something like:If the :option:`--pretty` option is passed to the command, ...
rbtools/commands/api.py (Diff revision 14) |
---|
This is missing a return type.
We're also not streaming to anything. We're just returning a string.
rbtools/commands/api.py (Diff revision 14) |
---|
You want a blank line before the statement and the loop, but not on the first inner line of the loop.
rbtools/commands/api.py (Diff revision 14) |
---|
This can be simplified massively by doing a couple things:
Have each HTTP method option (
--get
,--post
, etc.) have an action ofstore_const
, setting a central attribute (say,'http_method'
) to a value (get
,post
, etc.).Update this code to compute the correct function to call:
http_method = self.options.http_method if path.startswith(...): client_method_name = '%s_url' % http_method else: client_method_name = '%s_path' % http_method client_method = getattr(api_client, http_method_name) assert client_method is not None resource = client_method(...)
rbtools/commands/api.py (Diff revision 14) |
---|
startswith
takes a tuple, for convenience:if path.startswith(('http://', 'https://')):
rbtools/commands/api.py (Diff revision 14) |
---|
I think we want to avoid allowing anything unexpected to be a query argument. What if an API takes a query argument of
?get=...
?The old code did this, but it was a bad idea.
Let's instead add a
--query
parameter, or allow it on the URL itself.
rbtools/commands/api.py (Diff revision 14) |
---|
The above recommendation about the HTTP method options will eliminate all of this.
Now, that doesn't mean that it'll duplicate this behavior. The way it'll work is that the last of
--get
,--post
, etc. specified wins. That isn't ideal, but it's frankly good enough.If we really want to protect the user here, the right solution would be to make a new action that gets passed to options, and have it warn at that time, before we do all this work.
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+2691 -1491) |
rbtools/api/transport/sync.py (Diff revision 15) |
---|
F841 local variable 'body_dict' is assigned to but never used
rbtools/api/transport/sync.py (Diff revision 15) |
---|
F841 local variable 'file_path_list' is assigned to but never used
rbtools/api/transport/sync.py (Diff revision 15) |
---|
F841 local variable 'file_path_list' is assigned to but never used
rbtools/api/transport/sync.py (Diff revision 15) |
---|
F841 local variable 'file_path_list' is assigned to but never used
rbtools/api/transport/sync.py (Diff revision 15) |
---|
F841 local variable 'body_dict' is assigned to but never used
rbtools/api/transport/sync.py (Diff revision 15) |
---|
F841 local variable 'file_path_list' is assigned to but never used
rbtools/commands/api.py (Diff revision 15) |
---|
F841 local variable 'query_dict' is assigned to but never used
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 16 (+2753 -1563) |
rbtools/commands/api.py (Diff revision 16) |
---|
F841 local variable 'query_arguments' is assigned to but never used
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 17 (+2768 -1572) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 18 (+2771 -1575) |