This is an automatically generated e-mail. To reply, visit: http://reviewboard-tflink.rhcloud.com/r/40/

There are GPL headers missing from most of the new source files - please add them

blockerbugs/controllers/api/api.py (Diff revision 1)
19
api_bp = Blueprint('api', __name__,
Why did you name the blueprint api_bp when none of the other blueprints use a '_bp' suffix? with the variable API_VERSION that can be set here, wouldn't it be better to call it api_v0 instead or just api until such a time when we have multiple API versions?

blockerbugs/controllers/api/errors.py (Diff revision 1)
1
import httplib
api/errors is missing a GPL header

blockerbugs/controllers/api/iso8601.py (Diff revision 1)
48
class ParseError(Exception):
why are you duplicating this code instead of pulling it in as a dependency? Isn't it the same code as https://pypi.python.org/pypi/iso8601 and https://admin.fedoraproject.org/pkgdb/acls/name/python-iso8601

blockerbugs/models/spin.py (Diff revision 1)
class Spin(db.Model):
66
        if date_requested is None:
Is there a reason to make this change other than your sense of style? As I recall, most other model constructors do it the pre-change way

testing/test_validators.py (Diff revision 1)
20
        valid_integer2 = 12.3
this bothers me a little - is 12.3 really validated as an int?

testing/test_validators.py (Diff revision 1)
26
        #test max and min
this should be a separate test case, not just put into one

testing/test_validators.py (Diff revision 1)
36
    #
empty comment

testing/test_validators.py (Diff revision 1)
121
    def test_list(self):
wouldn't this be better as test_int_list?

A couple of general concerns I have about the code:
 - your tests are, in general, way too compact. There are way too many assertions and checks per test that make it much more difficult to diagnose errors if/when they happen. I understand that adhering blindly to a 'one assertion per test' rule is silly but I think you've taken it too far in the other direction. Additional testing/poking shouldn't be required to figure out what is failing in a test (ie, whether the optional or required elements of the dict validator are failing)

 - I'd like to see more explicit handling of the API version. I like having the version and I think it should stay, I just don't like hardcoding the 'v0' in all the tests without indicating that the test class is only for 'v0' - the file names shouldn't be generic if the api version they work with is specific.

Overall, I think it looks good. All the stuff I'm pointing out is minor stuff

- Tim Flink


On July 22nd, 2013, 12:04 p.m. UTC, Ilgiz Islamgulov wrote:

Review request for blockerbugs.
By Ilgiz Islamgulov.

Updated July 22, 2013, 12:04 p.m.

Bugs: 392
Repository: blockerbugs

Description

Add endpoints:
 - list bugs
 - list updates
 - list spins
 - create spin

Testing

Wrote test suites.
I've tested on my develop instance.

Diffs

  • testing/test_validators.py (PRE-CREATION)
  • testing/test_api.py (PRE-CREATION)
  • blockerbugs/models/spin.py (99891448e78c7168d540b479ffd7ef00ce1eec1d)
  • blockerbugs/controllers/api/validators.py (PRE-CREATION)
  • blockerbugs/controllers/api/iso8601.py (PRE-CREATION)
  • blockerbugs/controllers/api/errors.py (PRE-CREATION)
  • blockerbugs/controllers/api/api.py (PRE-CREATION)
  • blockerbugs/controllers/api/__init__.py (PRE-CREATION)
  • blockerbugs/__init__.py (c0f298462670e2ca00ba63fd8e722d2babbe5760)

View Diff