Encode non-string query parameters as strings

Review Request #10685 — Created Sept. 6, 2019 and submitted

Information

RBTools
release-1.0.x

Reviewers

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
Encode non-string query parameters as 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: 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.
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 …

chipx86chipx86

Also, I want to see testing on all supported Python versions (2.7, 3.5-3.7).

chipx86chipx86

Can you expand on this, to better define when the behavior changed?

chipx86chipx86

Wrapping in Testing Done is wrong. "hold" is probably not the right word there.

chipx86chipx86

This should be on release-1.0.x.

chipx86chipx86

Can you add unit tests for all the encoding possibilities?

chipx86chipx86

In "Testing Done", "Confirmed the following hold true" is still pretty weird phrasing. How about just "Confirmed the following on …

daviddavid

How about also adding encode_url_key, which forces to Unicode and does the replace()? That'll simplify this and make it a …

chipx86chipx86

Missing Returns and Raises.

chipx86chipx86

Okay, so I ran into this with Python porting work before. long is also valid -- but only on Python …

chipx86chipx86

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E113 unexpected indentation

reviewbotreviewbot

F841 local variable 'request' is assigned to but never used

reviewbotreviewbot
chipx86
  1. 
      
  2. I wonder if instead we should fix this during the request building, instead of here, so that we don't have to make a dozen similar changes (and so that customers using the API don't have to either).

  3. Can you expand on this, to better define when the behavior changed?

  4. 
      
chipx86
  1. 
      
  2. Also, I want to see testing on all supported Python versions (2.7, 3.5-3.7).

  3. 
      
brennie
chipx86
  1. 
      
  2. Wrapping in Testing Done is wrong.

    "hold" is probably not the right word there.

  3. This should be on release-1.0.x.

  4. Can you add unit tests for all the encoding possibilities?

  5. rbtools/api/request.py (Diff revision 2)
     
     
     

    How about also adding encode_url_key, which forces to Unicode and does the replace()? That'll simplify this and make it a little nicer to maintain.

  6. rbtools/api/request.py (Diff revision 2)
     
     
     
     

    Missing Returns and Raises.

  7. 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,)):

  8. 
      
brennie
david
  1. Looks like you marked Christian's issues as fixed but haven't posted a new revision.

  2. In "Testing Done", "Confirmed the following hold true" is still pretty weird phrasing. How about just "Confirmed the following on Python versions ..."?

  3. 
      
brennie
Review request changed

Commits:

Summary ID
Encode non-string query parameters as 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: 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.
58d0bb153548686a6c896d6b9c4a99219ce9eac7
Encode non-string query parameters as 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: 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.
f7a53e71fec932918554fda1e20da428b12e82b5

Diff:

Revision 3 (+196 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Commits:

Summary ID
Encode non-string query parameters as 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: 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.
f7a53e71fec932918554fda1e20da428b12e82b5
Encode non-string query parameters as 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: 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.
295e320552c17e18388150b902fb9861a9e22f1d

Diff:

Revision 4 (+196 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (7b7188e)
Loading...