Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geo commands #177

Merged
merged 8 commits into from Jan 5, 2017

Conversation

@polyn0m
Copy link
Contributor

commented Jan 3, 2017

Added GeoCommandsMixin for support Redis geo commands since redis 3.2.0

@codecov-io

This comment has been minimized.

Copy link

commented Jan 3, 2017

Current coverage is 97.03% (diff: 100%)

Merging #177 into master will increase coverage by 0.09%

@@             master       #177   diff @@
==========================================
  Files            49         51     +2   
  Lines          5933       6131   +198   
  Methods           0          0          
  Messages          0          0          
  Branches        445        462    +17   
==========================================
+ Hits           5751       5949   +198   
  Misses          139        139          
  Partials         43         43          

Powered by Codecov. Last update 6941cee...8a70c32

Copy link
Member

left a comment

Good job!
Please fix flake errors and see my comments.

"""
encoding = _NOTSET
if 'encoding' in kwargs:
encoding = kwargs.pop('encoding')

This comment has been minimized.

Copy link
@popravich

popravich Jan 4, 2017

Member

No need in such encoding parsing, it will be extracted from kwargs in execute method.

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 4, 2017

Author Contributor

Removed

for row in value:
res = []

if with_dist and with_coord and with_hash:

This comment has been minimized.

Copy link
@popravich

popravich Jan 4, 2017

Member

This looks overcomplicated. Lets make it simple.

First of all I think it would be nice to have these results wrapped in namedtuple
so users would not have to remember meaning of each index in a row:

GeoCoord = namedtuple('GeoCoord', 'longtitude latitude')
GeoRadius = namedtuple('GeoRadius', 'name distance hash coord')

def make_geo_radius(name, distance=None, hash=None, coord=None):
    if distance is not None:
        distance = float(distance)
    if hash is not None:
        hash = int(hash)
    if coord is not None:
        hash = GeoCoord(*map(float, coord))
    return GeoRadius(name, distance, hash, coord)

So geo_data_row can look like this:

...
for row in value:
    if isinstance(row, list):
        res_rows.append(make_geo_radius(*row))
    else:
        res_rows.append(row)
...

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 4, 2017

Author Contributor

Added types and rewrite function

@@ -73,6 +73,14 @@ def wait_convert(fut, type_):


@asyncio.coroutine
def wait_convert_with_opts(fut, type_, **opts):

This comment has been minimized.

Copy link
@popravich

popravich Jan 4, 2017

Member

No need in separate function, just add **kwargs or **opts to wait_convert

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 4, 2017

Author Contributor

Fixed

Copy link
Member

left a comment

few more notes



GeoCoord = namedtuple('GeoCoord', ('longitude', 'latitude'))
GeoMember = namedtuple('GeoRadius', ('member', 'dist', 'hash', 'coord'))

This comment has been minimized.

Copy link
@popravich

popravich Jan 4, 2017

Member

names should match

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 4, 2017

Author Contributor

@popravich what kind names must match? GeoMember -> GeoRadius?

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 4, 2017

Author Contributor

Ahhh, typos...

elif with_coord:
res.append(row[0])
res.append([float(row[1][0]), float(row[1][1])])
member, distance, coord, hash_ = None, None, None, None

This comment has been minimized.

Copy link
@popravich

popravich Jan 4, 2017

Member

Ok, my previous example was incorrect.
But I realy don't like this long if-elif noodles.
How about this:

if isinstance(row, list):
    name = row.pop(0)
    dist = hash = coord = None
    if with_dist:
        dist = float(row.pop(0))
    if with_hash:
        hash = int(row.pop(0))
    if with_coord:
        coord = GeoCoord(*map(float, row.pop(0)))
    return GeoMember(name, dist, hash, coord)
)
assert res == 2

res = yield from redis.georadius(
'geodata', 15, 37, 200, 'km', encoding='utf-8'
)
assert res == [
['Palermo'], ['Catania']

This comment has been minimized.

Copy link
@popravich

popravich Jan 4, 2017

Member

I think georadius* commands without with_* flags must behave same as Redis — return list of strings,
not list of lists of strings or namedtuples.

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 4, 2017

Author Contributor

i think command must return same result - list of namedtuples, because no reason for return list of strings, it's same record without additional data. And now command always return list of GeoMember.

This comment has been minimized.

Copy link
@popravich

popravich Jan 5, 2017

Member

@polyn0m, I was thinking about it and still sceptical.
We must follow Redis georadius return convention, ie:
return list of strings when no withcoord / withhash / withdist flags are set;
return list of (named)tuples when any of above flags are set.

In cases when no WITH* flags are set distance, coordinate and hash fields
of tuple would be empty and the only usable field would be name.
User would need to convert such list of one-item-tuples to plain list
to use it (for instance to do join — ', '.join(['Palermo', 'Catania']),
or to pass this results to other DB query...)

In case we always return list of tuples there would be situations when double conversion
happen — aioredis convert plain list to list of tuples; then user converts list of tuples to plain list.
I think this would be uncommon case but it (returning plain list of strings) would
make code more readable and simplier (both library- and user- sides).

@popravich

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

build is still failing, please fix flake errors.

@polyn0m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2017

Manual run result http://joxi.ru/aD2PGWpiNld9m3

assert res == [
GeoCoord(longitude=15.087267458438873, latitude=37.50266842333162),
GeoCoord(longitude=13.36138933897018433, latitude=38.11555639549629859)
]

This comment has been minimized.

Copy link
@popravich

popravich Jan 5, 2017

Member

Add test (and fix) for the following case:

res = yield from redis.geopos('geodata', 'Catania', 'NonExistingMember', 'Palermo')

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 5, 2017

Author Contributor

Added test_geo_not_exist_members test

member='Catania', dist=166.2742, hash=3479447370796909,
coord=GeoCoord(15.087267458438873, 37.50266842333162)
)
]

This comment has been minimized.

Copy link
@popravich

popravich Jan 5, 2017

Member

Please add tests for each command without encoding

This comment has been minimized.

Copy link
@polyn0m

polyn0m Jan 5, 2017

Author Contributor

Added *_binary tests


def georadius(self, key, longitude, latitude, radius, unit='m',
dist=False, hash=False, coord=False,
count=None, sort=None, encoding=_NOTSET):

This comment has been minimized.

Copy link
@popravich

popravich Jan 5, 2017

Member

Please keep with_ prefixes for dist, hash and coord parameters.

matching a given maximum distance from a point
:raises TypeError: radius is not float or int
:raises TypeError: count is not float or int

This comment has been minimized.

Copy link
@popravich

popravich Jan 5, 2017

Member

typo: count can be int only


def georadiusbymember(self, key, member, radius, unit='m',
dist=False, hash=False, coord=False,
count=None, sort=None, encoding=_NOTSET):

This comment has been minimized.

Copy link
@popravich

popravich Jan 5, 2017

Member

See comments above regarding with_ prefix and "count typo".

polyn0m added 2 commits Jan 5, 2017
@popravich popravich merged commit d586720 into aio-libs:master Jan 5, 2017
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 96.93%)
Details
codecov/project 97.03% (+0.09%) compared to 6941cee
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@popravich

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Good job! Thanks

@polyn0m

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2017

You are welcome

@popravich popravich modified the milestone: v0.3 Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.