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

On August 2nd, 2013, 2:38 p.m. UTC, Tim Flink wrote:

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?
Just 'api' name conflict with module name. As for me better  to call it api_v0.

On August 2nd, 2013, 2:38 p.m. UTC, Tim Flink wrote:

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
I thought that having include one file is better than having dependency. Will use this package.

On August 2nd, 2013, 2:38 p.m. UTC, Tim Flink wrote:

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
datetime.utcnow() is called once when it will load. I guess that date_requested should have time then model created.

In [2]: def foo(d=datetime.utcnow()):
    return d
   ...: 

In [3]: foo()
Out[3]: datetime.datetime(2013, 8, 2, 18, 37, 20, 876235)

In [4]: %date -u
ERROR: Line magic function `%date` not found.
In [5]: !date -u
Fri Aug  2 18:37:43 UTC 2013

In [6]: foo()
Out[6]: datetime.datetime(2013, 8, 2, 18, 37, 20, 876235)

On August 2nd, 2013, 2:38 p.m. UTC, Tim Flink wrote:

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?
Looks like odd. Really float value should not be valid int.

On August 2nd, 2013, 2:38 p.m. UTC, Tim Flink wrote:

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
Thank you for your review. 

I'm totally agree with your comments about tests. Also message with assertion should be useful.


Looks like there should be generic version of api tests and separated tests for specific api version. In my mind all api tests are generic now.

- Ilgiz


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