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:
-
Pass query args as strings in rbt statusEncode non-string query parameters as strings
- Description:
-
~ The status command was passing the repository ID as an integer to
~ get_review_requests()
, which was fine in the past, but it requires all~ argument values to be strings. ~ 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. - Testing Done:
-
~ With this patch applied, rbt status works.
~ Confirmed the following hold on Python verisons 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. - Commits:
-
Summary ID 744b98f0b219c78b4dbb6dd2c2ccf5db5b61613e 58d0bb153548686a6c896d6b9c4a99219ce9eac7 - Diff:
-
Revision 2 (+66 -2)
Checks run (2 succeeded)
-
-
-
-
-
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. -
-
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:
-
~ Confirmed the following hold on Python verisons 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
~ 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.
- post is able to create review requests with commit history. - With this patch applied,
- Commits:
-
Summary ID 58d0bb153548686a6c896d6b9c4a99219ce9eac7 f7a53e71fec932918554fda1e20da428b12e82b5 - Diff:
-
Revision 3 (+196 -6)
- Commits:
-
Summary ID f7a53e71fec932918554fda1e20da428b12e82b5 295e320552c17e18388150b902fb9861a9e22f1d - Diff:
-
Revision 4 (+196 -6)
- Commits:
-
Summary ID 295e320552c17e18388150b902fb9861a9e22f1d 6e6c34885ada13f65fed674d12833b0051c95e39 - Diff:
-
Revision 5 (+244 -6)