Encode non-string query parameters as strings
Review Request #10685 — Created Sept. 6, 2019 and submitted
There are a few cases where we are passing non-string arguments to
methods that make API calls, which was fine in the past. However, we are
now assuming that all arguments are strings and erroring out if they are
not.Instead, we now make a best-case attempt to serialize values to strings
and produces a better error message when this process fails.
Confirmed the following hold true on Python versions 2.7, 3.5, 3.6, and
3.7:
- With this patch applied,
rbt status
prints the list of status, where
previously it would throw an exception due to attempting to stringify
non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697),
rbt post
is able to create review requests with commit history.
Summary | ID |
---|---|
6e6c34885ada13f65fed674d12833b0051c95e39 |
Description | From | Last Updated |
---|---|---|
I wonder if instead we should fix this during the request building, instead of here, so that we don't have … |
chipx86 | |
Also, I want to see testing on all supported Python versions (2.7, 3.5-3.7). |
chipx86 | |
Can you expand on this, to better define when the behavior changed? |
chipx86 | |
Wrapping in Testing Done is wrong. "hold" is probably not the right word there. |
chipx86 | |
This should be on release-1.0.x. |
chipx86 | |
Can you add unit tests for all the encoding possibilities? |
chipx86 | |
In "Testing Done", "Confirmed the following hold true" is still pretty weird phrasing. How about just "Confirmed the following on … |
david | |
How about also adding encode_url_key, which forces to Unicode and does the replace()? That'll simplify this and make it a … |
chipx86 | |
Missing Returns and Raises. |
chipx86 | |
Okay, so I ran into this with Python porting work before. long is also valid -- but only on Python … |
chipx86 | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E113 unexpected indentation |
reviewbot | |
F841 local variable 'request' is assigned to but never used |
reviewbot |
Summary: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+66 -2) |
Checks run (2 succeeded)
-
-
-
-
-
rbtools/api/request.py (Diff revision 2) How about also adding
encode_url_key
, which forces to Unicode and does thereplace()
? That'll simplify this and make it a little nicer to maintain. -
-
rbtools/api/request.py (Diff revision 2) Okay, so I ran into this with Python porting work before.
long
is also valid -- but only on Python 2.x. Fortunately,six
has our back. So we actually want:`python if isinstance(value, six.integer_types + (float,)):
Testing Done: |
|
---|
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+196 -6) |
Checks run (1 failed, 1 succeeded)
flake8
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+196 -6) |
Checks run (1 failed, 1 succeeded)
flake8
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+244 -6) |