[LIB-653] GeoPoint re work, add tests, add possibility to filter; #191
Conversation
|
@dancio will be nice if you would take a look. |
|
lgtm |
syncano/models/fields.py
Outdated
| class GeoPoint(Field): | ||
| class GeoPointStruct(object): | ||
|
|
||
| def __init__(self, latitude=None, longitude=None, distance=None, unit=None): |
There was a problem hiding this comment.
Point is a point, It doesn't have distance, or even radius. What is unit ?
There was a problem hiding this comment.
Also if latitude and longitude are required they should not have default values.
… add possibility to define lookup near in such way: GeoLookup(GeoPoint(lat, lng), distance, unit)
|
@forgems test are running - and will probably fail, but you can take a look into correctness :) |
|
|
||
| if not isinstance(value, GeoPointStruct): | ||
| raise SyncanoValueError('Expected an GeoPointStruct') | ||
| if not isinstance(value, GeoPoint): |
There was a problem hiding this comment.
This kind of checks can be done with assertions like that https://wiki.python.org/moin/UsingAssertionsEffectively:
assert isinstance(value, GeoPoint), 'Expected GeoPoint'
There was a problem hiding this comment.
Using asserts in code that is meant to be normally used (outside of tests) is baaad. Especially for validations.
python -O and suddenly my validations do not work ;)
There was a problem hiding this comment.
The last time I was running python with -O flag was in 2007. It doesn't give you any kind of advantage :) Maybe if you run your tests with -O flag, then it would be a problem, but otherwise IMHO it's ok. And it's the official way for type assertions.
There was a problem hiding this comment.
Well, not exactly. It's official in terms of places where the object is actually used. This is a validation function so these are two different things to me. Never seen an assert in a validator of any kind. Although it's likely that main reason for this is the fact that we want a specific Exception class that can be handled rather than AssertionError which suggests something went very wrong.
Anyway, python promotes duck-typing heavily and doesn't like using isinstance. In my opinion that's why they are ok with asserts officially. But coming from C/C++ background, depending on asserts is well, ugly.
Also, kind of unrelated - it's pretty ridiculous that they changed in 3.x print into a function but assert is still a statement.
There was a problem hiding this comment.
Me not like asserts in such context - so leave it that way.
No description provided.