Encode non-string query parameters as strings

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

brennie
RBTools
master
10695
rbtools

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
Encode non-string query parameters as strings
Description From Last Updated

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

chipx86chipx86

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

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

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
chipx86
  1. 
      
  2. Can you expand on this, to better define when the behavior changed?

  3. 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).

  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
Review request changed

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.

Loading...