Summary

Improve support for fine-grained text types in the API.

Review Request #6458 — Created Oct. 17, 2014 and submitted Oct. 19, 2014, 1:59 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Commit

db90d78...

Reviewers

Groups

reviewboard

People

Description

This updates the various resources to support fine-grained setting of
text types per-field, to list the text types, and to optionally return
the raw values along with the forced values (if using force-text-type=
and include-raw-text-fields=).

Support for setting text_type= is still allowed on the resources that
have split this field out. It will act as if individual *_text_type=
values were passed. However, the returned text_type value in the payload
is now hard-coded to null.

New text.per_field_text_types and text.can_include_raw_values
capabilities have been added.

This is part of a set of changes to allow for per-field optional for
Markdown. It requires the other set of changes.

Testing Done

Unit tests pass.

Tested all the new support through the UI, using upcoming changes.

Issues

Description	From	Last Updated
Col: 16 E127 continuation line over-indented for visual indent	reviewbot	Oct. 17, 2014, 5:40 p.m.
local variable 'old_rich_text' is assigned to but never used	reviewbot	Oct. 17, 2014, 5:40 p.m.
redefinition of unused 'test_post_with_text_type_markdown' from line 9	reviewbot	Oct. 17, 2014, 5:40 p.m.
redefinition of unused 'test_post_with_body_top_text_type_markdown' from line 27	reviewbot	Oct. 17, 2014, 5:40 p.m.
redefinition of unused 'test_post_with_body_bottom_text_type_markdown' from line 45	reviewbot	Oct. 17, 2014, 5:40 p.m.
redefinition of unused 'test_put_with_text_type_markdown' from line 162	reviewbot	Oct. 17, 2014, 5:41 p.m.
redefinition of unused 'test_put_with_body_top_text_type_markdown' from line 180	reviewbot	Oct. 17, 2014, 5:41 p.m.
redefinition of unused 'test_put_with_body_bottom_text_type_markdown' from line 198	reviewbot	Oct. 17, 2014, 5:41 p.m.
Shouldn't this be treated as an error?	david	Oct. 19, 2014, 1:31 p.m.
This loop is now pretty comical. Can you flatten it out?	david	Oct. 19, 2014, 1:31 p.m.
Would it make sense to raise NotImplementedError?	david	Oct. 19, 2014, 1:35 p.m.
Would it make sense to raise NotImplementedError?	david	Oct. 19, 2014, 1:35 p.m.
Same here.	david	Oct. 19, 2014, 1:35 p.m.
And here?	david	Oct. 19, 2014, 1:35 p.m.
And here?	david	Oct. 19, 2014, 1:36 p.m.
This should use "text type" instead of "text mode"	david	Oct. 19, 2014, 1:32 p.m.
This should use "text type" instead of "text mode"	david	Oct. 19, 2014, 1:32 p.m.
You could put the ( on the first line.	david	Oct. 19, 2014, 1:32 p.m.
Shouldn't this be passing in close_description_rich_text?	david	Oct. 19, 2014, 1:33 p.m.
I'm going to stop marking these, but if you decide on using NotImplementedError, can you do it throughout?	david	Oct. 19, 2014, 1:36 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/test_review.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/models/review_request.py
    reviewboard/webapi/tests/mixins_review.py
    reviewboard/webapi/resources/base_review.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/webapi/resources/review_reply.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/test_review.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/models/review_request.py
    reviewboard/webapi/tests/mixins_review.py
    reviewboard/webapi/resources/base_review.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/webapi/resources/review_reply.py

reviewboard/webapi/mixins.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 16
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/base_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'old_rich_text' is assigned to but never used
```
reviewboard/webapi/tests/mixins_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'test_post_with_text_type_markdown' from line 9
```
reviewboard/webapi/tests/mixins_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'test_post_with_body_top_text_type_markdown' from line 27
```
reviewboard/webapi/tests/mixins_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'test_post_with_body_bottom_text_type_markdown' from line 45
```
reviewboard/webapi/tests/mixins_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'test_put_with_text_type_markdown' from line 162
```
reviewboard/webapi/tests/mixins_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'test_put_with_body_top_text_type_markdown' from line 180
```
reviewboard/webapi/tests/mixins_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 redefinition of unused 'test_put_with_body_bottom_text_type_markdown' from line 198
```

Testing Done:: Unit tests pass.
+
+
Tested all the new support through the UI, using upcoming changes.

Change Summary:

Fixed Review Bot complaints.

Commit:

07d2caf48e35269aa92eb896fd5e1e91510bb326

00832d0c92857258c095d0e6676c0218c2f88298

Diff:

Revision 2 (+708 -598)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/webapi/mixins.py
	reviewboard/webapi/server_info.py
	reviewboard/webapi/resources/base_comment.py
	reviewboard/webapi/resources/base_review.py
	reviewboard/webapi/resources/review_reply.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/test_review.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/models/review_request.py
    reviewboard/webapi/tests/mixins_review.py
    reviewboard/webapi/resources/base_review.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/webapi/resources/review_reply.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/test_review.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/models/review_request.py
    reviewboard/webapi/tests/mixins_review.py
    reviewboard/webapi/resources/base_review.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/webapi/resources/review_reply.py

reviewboard/webapi/mixins.py (Diff revision 2)

The issue has been dropped. Show all issues

Shouldn't this be treated as an error?

chipx86 Oct. 19, 2014, 1:33 p.m.

Theoretically yes, but we don't have any way to bubble up an error from here.

reviewboard/webapi/resources/base_review.py (Diff revision 2)

The issue has been resolved. Show all issues

This loop is now pretty comical. Can you flatten it out?

chipx86 Oct. 19, 2014, 1:33 p.m.

Yeah. I kept it in case we added new fields later, to keep the pattern the same with other resources. Not much point though.

reviewboard/webapi/resources/review_reply.py (Diff revision 2)

The issue has been dropped. Show all issues

Would it make sense to raise NotImplementedError?

chipx86 Oct. 19, 2014, 1:33 p.m.
```
Sure.
```

chipx86 Oct. 19, 2014, 1:35 p.m.

Oops, I remember why I return None.
These actually do get serialized. We just override them. The values are useless, but something must be returned. If an error is raised, serialization fails.
Switching back to None.

reviewboard/webapi/resources/review_reply.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Would it make sense to raise NotImplementedError?
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Same here.
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been dropped. Show all issues
```
And here?
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been dropped. Show all issues
```
And here?
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should use "text type" instead of "text mode"
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should use "text type" instead of "text mode"
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
You could put the ( on the first line.
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Shouldn't this be passing in close_description_rich_text?
```

reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)

The issue has been dropped. Show all issues

I'm going to stop marking these, but if you decide on using NotImplementedError, can you do it throughout?

Change Summary:


Fixed some wording in doc strings.
Simplified the handling of ship_it in base_review.py.
Passed the correct value for close_description_rich_text.
Cleaned up a line continuation.

Commit:

00832d0c92857258c095d0e6676c0218c2f88298

db90d785ab06c7eca6d586a76c5e8cdd6241d96d

Diff:

Revision 3 (+709 -602)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/webapi/mixins.py
	reviewboard/webapi/server_info.py
	reviewboard/webapi/resources/base_comment.py
	reviewboard/webapi/resources/base_review.py
	reviewboard/webapi/resources/review_reply.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/test_review.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/models/review_request.py
    reviewboard/webapi/tests/mixins_review.py
    reviewboard/webapi/resources/base_review.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/webapi/resources/review_reply.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/test_review.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/models/review_request.py
    reviewboard/webapi/tests/mixins_review.py
    reviewboard/webapi/resources/base_review.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/webapi/resources/review_reply.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to markdown-redesign (b0d894f)