Summary

Properly decode API errors from UTF-8

Review Request #10697 — Created Sept. 6, 2019 and submitted Jan. 7, 2020, 1:03 a.m.

Information

Owner

brennie

Repository

RBTools

Branch

release-1.0.x

Bugs

Depends On

~~10696~~

Reviewers

Groups

rbtools

People

Description

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

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.	a098a9b4bdb8284154ddb245796d10d0fac6f1ee	Barret Rennie

Issues

Description	From	Last Updated
Wrapping in Testing Done is wrong. Make sure you're not wrapping in the middle of a literal.	chipx86	Sept. 7, 2019, 3:58 p.m.
This should land on release-1.0.x, rather than mater, I think?	chipx86	Sept. 10, 2019, 1:33 p.m.
Typo in testing done: verisons -> versions.	david	Sept. 27, 2019, 2:03 p.m.
In "Testing Done", "Confirmed the following hold true" is still pretty weird phrasing. How about just "Confirmed the following on …	david	Sept. 27, 2019, 2:03 p.m.
Can you give this a docstring while here?	chipx86	Sept. 10, 2019, 1:35 p.m.
You can use rbtools.utils.encoding.force_unicode() for this.	chipx86	Sept. 10, 2019, 1:34 p.m.
Okay, higher-level. The newlines that we have in here seem to be in the wrong place. We're joining the entries …	chipx86	Sept. 10, 2019, 1:34 p.m.
Also, no u prefix.	chipx86	Sept. 10, 2019, 1:34 p.m.
F821 undefined name 'msg'	reviewbot	Sept. 10, 2019, 1:37 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

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

chipx86 Sept. 7, 2019, 1:25 p.m.
```
Also, "hold" seems wrong.
```

The issue has been resolved. Show all issues

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

rbtools/api/errors.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you give this a docstring while here?
```

rbtools/api/errors.py (Diff revision 1)

The issue has been resolved. Show all issues

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

rbtools/commands/post.py (Diff revision 1)

The issue has been resolved. 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).

rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Also, no u prefix.
```

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

Diff:

Revision 2 (+40 -24)

Show changes

	rbtools/api/errors.py
	rbtools/commands/post.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/api/errors.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'msg'
```

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.	943cd2af8af400d7ca9cdf44ac47d13c69b6f3db	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.	a098a9b4bdb8284154ddb245796d10d0fac6f1ee	Barret Rennie

Diff:

Revision 3 (+40 -24)

Show changes

	rbtools/api/errors.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Typo in testing done: verisons -> versions.

The issue has been resolved. 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 ..."?

Testing Done:: ~
Confirmed the following hold true on Python verisons 2.7, 3.5, 3.6, and
~
Confirmed the on Python versions 2.7, 3.5, 3.6, and 3.7:
- 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.

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-1.0.x (bdd8260)