Properly decode API errors from UTF-8

Review Request #10697 — Created Sept. 7, 2019 and submitted

Information

RBTools
release-1.0.x

Reviewers

When we received an error message from the Review Board API, if we
didn't have a pre-generated string for that error code locally we would
include the error body verbatim. We did this by attempting to cast it to
a string and encode it to utf-8, but that only works on Python 2 where
str is actually bytes. On Python 3, an error is raised becuase you are
not allowed to decode a bytes object into text without a codec.

Instead, we now return text from the API errors when calling str(e) on
them, instead of returning their body verbatim.

While I was here, I also removed a bunch of unnecessary unicode
literals. We are using unicode_literals everywhere so they were
redundant.

Confirmed the on Python versions 2.7, 3.5, 3.6, and 3.7:

  • With this entire patch stack (/r/10685, /r/10695, and /r/10696),
    rbt post is able to create review requests with commit history.
Summary ID Author
Properly decode API errors from UTF-8
When we received an error message from the Review Board API, if we didn't have a pre-generated string for that error code locally we would include the error body verbatim. We did this by attempting to cast it to a string and encode it to utf-8, but that only works on Python 2 where str is actually bytes. On Python 3, an error is raised becuase you are not allowed to decode a bytes object into text without a codec. Instead, we now return text from the API errors when calling `str(e)` on them, instead of returning their body verbatim. While I was here, I also removed a bunch of unnecessary unicode literals. We are using `unicode_literals` everywhere so they were redundant. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this entire patch stack (/r/10685, /r/10695, and /r/10696), `rbt post` is able to create review requests with commit history.
a098a9b4bdb8284154ddb245796d10d0fac6f1ee Barret Rennie
Description From Last Updated

Wrapping in Testing Done is wrong. Make sure you're not wrapping in the middle of a literal.

chipx86chipx86

This should land on release-1.0.x, rather than mater, I think?

chipx86chipx86

Typo in testing done: verisons -> versions.

daviddavid

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

daviddavid

Can you give this a docstring while here?

chipx86chipx86

You can use rbtools.utils.encoding.force_unicode() for this.

chipx86chipx86

Okay, higher-level. The newlines that we have in here seem to be in the wrong place. We're joining the entries …

chipx86chipx86

Also, no u prefix.

chipx86chipx86

F821 undefined name 'msg'

reviewbotreviewbot
chipx86
  1. 
      
  2. Show all issues

    Wrapping in Testing Done is wrong. Make sure you're not wrapping in the middle of a literal.

    1. Also, "hold" seems wrong.

  3. Show all issues

    This should land on release-1.0.x, rather than mater, I think?

  4. rbtools/api/errors.py (Diff revision 1)
     
     
    Show all issues

    Can you give this a docstring while here?

  5. rbtools/api/errors.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    You can use rbtools.utils.encoding.force_unicode() for this.

  6. rbtools/commands/post.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Okay, higher-level.

    The newlines that we have in here seem to be in the wrong place. We're joining the entries together with a newline when raising CommandError. If we're aiming to create blank lines between things, which I think is the purpose, let's strip away \n from these strings and just join with \n\n instead.

    We also should probably not be wrapping for error code 219. Wrapping to the terminal width is fine if we're going to do that, we should do it in the handler for CommandError (which doesn't need to happen in this change, but let's fix the strings anyway).

  7. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues

    Also, no u prefix.

  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 entire patch stack (/r/10685, /r/10695, and /r/10696), rbt
~   post is able to create review requests with commit history.

  ~

Confirmed the following hold true on Python verisons 2.7, 3.5, 3.6, and

  ~ 3.7:

  ~
  +
  • With this entire patch stack (/r/10685, /r/10695, and /r/10696),
    rbt post is able to create review requests with commit history.
Commits:
Summary ID Author
Properly decode API errors from UTF-8
When we received an error message from the Review Board API, if we didn't have a pre-generated string for that error code locally we would include the error body verbatim. We did this by attempting to cast it to a string and encode it to utf-8, but that only works on Python 2 where str is actually bytes. On Python 3, an error is raised becuase you are not allowed to decode a bytes object into text without a codec. Instead, we now return text from the API errors when calling `str(e)` on them, instead of returning their body verbatim. While I was here, I also removed a bunch of unnecessary unicode literals. We are using `unicode_literals` everywhere so they were redundant. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this entire patch stack (/r/10685, /r/10695, and /r/10696), `rbt post` is able to create review requests with commit history.
b753f871393357af40be95e3341d5a8661ba7f24 Barret Rennie
Properly decode API errors from UTF-8
When we received an error message from the Review Board API, if we didn't have a pre-generated string for that error code locally we would include the error body verbatim. We did this by attempting to cast it to a string and encode it to utf-8, but that only works on Python 2 where str is actually bytes. On Python 3, an error is raised becuase you are not allowed to decode a bytes object into text without a codec. Instead, we now return text from the API errors when calling `str(e)` on them, instead of returning their body verbatim. While I was here, I also removed a bunch of unnecessary unicode literals. We are using `unicode_literals` everywhere so they were redundant. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this entire patch stack (/r/10685, /r/10695, and /r/10696), `rbt post` is able to create review requests with commit history.
943cd2af8af400d7ca9cdf44ac47d13c69b6f3db Barret Rennie

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. Show all issues

    Typo in testing done: verisons -> versions.

  3. Show all issues

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

  4. 
      
brennie
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (bdd8260)