-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/#review56
-----------------------------------------------------------
There are GPL headers missing from most of the new source files - please add them
blockerbugs/controllers/api/api.py
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment80>
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
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment78>
api/errors is missing a GPL header
blockerbugs/controllers/api/iso8601.py
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment81>
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
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment82>
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
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment85>
this bothers me a little - is 12.3 really validated as an int?
testing/test_validators.py
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment84>
this should be a separate test case, not just put into one
testing/test_validators.py
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment86>
empty comment
testing/test_validators.py
<
http://reviewboard-tflink.rhcloud.com/r/40/#comment87>
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 22, 2013, 12:04 p.m., Ilgiz Islamgulov wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/40/
-----------------------------------------------------------
(Updated July 22, 2013, 12:04 p.m.)
Review request for blockerbugs.
Bugs: 392
https://fedorahosted.org/fedora-qa/ticket/392
Repository: blockerbugs
Description
-------
Add endpoints:
- list bugs
- list updates
- list spins
- create spin
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
Diff:
http://reviewboard-tflink.rhcloud.com/r/40/diff/
Testing
-------
Wrote test suites.
I've tested on my develop instance.
Thanks,
Ilgiz Islamgulov