Summary

Changed from hardcoded http codes to using constants from httplib

Review Request #7625 — Created Sept. 13, 2015 and submitted Sept. 20, 2015, 10:33 a.m.

Information

Owner

ahache

Repository

RBTools

Branch

master

Bugs

3841

Depends On

Reviewers

Groups

rbtools, reviewboard, students

People

Description

Changed from hardcoded 304 and 401 http codes to using the NOT_MODIFIED and UNAUTHORIZED constants from httplib for better readability.

Testing Done

RBTools nose test results - all passed fine

Ran 166 tests in 3.966s
OK (SKIP=50)

Issues

Description	From	Last Updated
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	Sept. 13, 2015, 12:35 p.m.
Col: 25 E127 continuation line over-indented for visual indent	reviewbot	Sept. 13, 2015, 12:38 p.m.
I think we want to keep 200 and 300 as numbers, and only use the symbolic error for NOT_MODIFIED.	david	Sept. 14, 2015, 9:46 a.m.
Don't add this line (io and json are still standard-library modules).	david	Sept. 16, 2015, 6:06 a.m.

Tool: Pyflakes
Processed Files:
    rbtools/api/request.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/request.py

rbtools/api/request.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```

Testing Done:

~		Not sure if it was necessary, but ran tests in the api/transport directory.
	~	Not sure if it was necessary, but ran tests in the api directory.

Change Summary:

Fixed max character issue as found by reviewbot

Diff:

Revision 2 (+4 -2)

Show changes

rbtools/api/request.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/request.py



Tool: Pyflakes
Processed Files:
    rbtools/api/request.py

rbtools/api/request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 25
 E127 continuation line over-indented for visual indent
```

Change Summary:

Fixed over indentation found by reviewbot

Diff:

Revision 3 (+4 -2)

Show changes

rbtools/api/request.py

Tool: Pyflakes
Processed Files:
    rbtools/api/request.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/request.py

Diff:

Revision 4 (+29 -6)

Show changes

	rbtools/api/request.py
	rbtools/api/transport/sync.py
	rbtools/commands/__init__.py

Tool: Pyflakes
Processed Files:
    rbtools/api/transport/sync.py
    rbtools/api/request.py
    rbtools/commands/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/transport/sync.py
    rbtools/api/request.py
    rbtools/commands/__init__.py

Diff:

Revision 5 (+4 -2)

Show changes

rbtools/api/request.py

Tool: Pyflakes
Processed Files:
    rbtools/api/request.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/request.py

rbtools/api/request.py (Diff revision 5)

The issue has been resolved. Show all issues

I think we want to keep 200 and 300 as numbers, and only use the symbolic error for NOT_MODIFIED.

Diff:

Revision 6 (+5 -3)

Show changes

rbtools/api/request.py

Tool: Pyflakes
Processed Files:
    rbtools/api/request.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/request.py

```
Looking pretty close now!
```

rbtools/api/request.py (Diff revision 6)

The issue has been resolved. Show all issues

Don't add this line (io and json are still standard-library modules).

ahache Sept. 16, 2015, 6:04 a.m.

Oh interesting. I left the space there because when looking a at the diff view between revision 0 and 1, it shows a space on the original. But looking at the master branch I can see that there was no space.
Gonna remove that!

Diff:

Revision 7 (+4 -3)

Show changes

rbtools/api/request.py

Tool: Pyflakes
Processed Files:
    rbtools/api/request.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/api/request.py

Groups:

+	students

<p>We don't need the issue number in the summary. Other than that it looks fine.</p>

AH

ahache Sept. 18, 2015, 3:43 p.m.
```
Updated!
```

Description:

~		Issue 3841: Changed from hardcoded http codes to using constants from httplib
	~	Changed from hardcoded http codes to using constants from httplib

Make sure to follow our guide on choosing a good summary and description: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

Your summary should never mention bug numbers (those will be provided automatically in the description when the change lands), and your descriptions should explain the why and how.

Summary:

-	Issue 3841: Changed from hardcoded http codes to using constants from httplib
+	Changed from hardcoded http codes to using constants from httplib

Description:

~		Changed from hardcoded http codes to using constants from httplib
	~	Changed from hardcoded 304 and 401 http codes to using the NOT_MODIFIED and UNAUTHORIZED constants from httplib for better readability.

Looks good.

Also, regarding the description, it's always good to run tests for every change, no matter what :)

brennie Sept. 18, 2015, 4:45 p.m.

Running all the tests is usually best practice.

Hey Anthony, would you mind running the tests with this patch to ensure all is well, and then posting the results in Testing Done?

Testing Done:

~		Not sure if it was necessary, but ran tests in the api directory.
	~	RBTools nose test results - all passed fine
	+
	+
	+
	+	Ran 166 tests in 3.966s
	+
	+	OK (SKIP=50)

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (72ccc5d)