Encode non-string query parameters as strings

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

brennie
RBTools
release-1.0.x
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

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. 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
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
-
Encode non-string query parameters as strings
+
Encode non-string query parameters as strings

Diff:

Revision 3 (+196 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Commits:

Summary
-
Encode non-string query parameters as strings
+
Encode non-string query parameters as strings

Diff:

Revision 4 (+196 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Change Summary:

This was posted while master was checked out because it was originally posted as a DVCS change and release-1.0.x cannot post to dvcs changes.

Branch:

-master
+release-1.0.x
david
  1. Ship It!
  2. 
      
Loading...