Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(179)

Issue 322730043: trytond-gis: Initial commit

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 1 week ago by nicoe
Modified:
1 month ago
Reviewers:
ced
Visibility:
Public.

Patch Set 1 #

Total comments: 12

Patch Set 2 : Fix comments and move regexp #

Patch Set 3 : rename abstract_type and move regexp to prevent circular imports #

Patch Set 4 : Add forgotten database.py #

Patch Set 5 : Add forgotten database.py #

Total comments: 21

Patch Set 6 : Use setUpModule to populate the module list and register the testing module #

Patch Set 7 : create the postgis extension on the database creation #

Patch Set 8 : Use setuptools find_packages to find packages #

Patch Set 9 : Use geojson instead of dbgis #

Patch Set 10 : remove comment #

Total comments: 24

Patch Set 11 : Use geomet, add tests, move stuffs in cont.py and sql.py #

Total comments: 19
Unified diffs Side-by-side diffs Delta from patch set Stats (+1150 lines, -0 lines) Patch
A COPYRIGHT View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -0 lines 0 comments Download
A LICENSE View 1 2 3 4 5 6 7 8 1 chunk +674 lines, -0 lines 0 comments Download
A MANIFEST.in View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
A README View 1 2 3 4 5 6 7 8 9 10 1 chunk +8 lines, -0 lines 1 comment Download
A setup.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +49 lines, -0 lines 1 comment Download
A tox.ini View 1 2 3 4 5 6 7 8 1 chunk +13 lines, -0 lines 0 comments Download
A trytond_gis/__init__.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +17 lines, -0 lines 2 comments Download
A trytond_gis/const.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +8 lines, -0 lines 0 comments Download
A trytond_gis/fields.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +81 lines, -0 lines 4 comments Download
A trytond_gis/postgis/__init__.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 1 comment Download
A trytond_gis/postgis/database.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +62 lines, -0 lines 2 comments Download
A trytond_gis/postgis/table.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +73 lines, -0 lines 1 comment Download
A trytond_gis/sql.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +10 lines, -0 lines 0 comments Download
A trytond_gis/tests/__init__.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 1 comment Download
A trytond_gis/tests/test_fields.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +107 lines, -0 lines 4 comments Download
A trytond_gis/tests/test_gis/__init__.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +12 lines, -0 lines 1 comment Download
A trytond_gis/tests/test_gis/gis.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +14 lines, -0 lines 1 comment Download

Messages

Total messages: 19
nicoe
7 months, 1 week ago (2017-03-17 13:53:08 UTC) #1
ced
https://codereview.appspot.com/322730043/diff/1/setup.py File setup.py (right): https://codereview.appspot.com/322730043/diff/1/setup.py#newcode10 setup.py:10: setup(name='trytond_gis', Maybe better: trytond_backend_gis https://codereview.appspot.com/322730043/diff/1/setup.py#newcode14 setup.py:14: url='http://trytond-gis.b2ck.com/', I'm wondering ...
7 months, 1 week ago (2017-03-17 14:36:10 UTC) #2
nicoe
https://codereview.appspot.com/322730043/diff/1/setup.py File setup.py (right): https://codereview.appspot.com/322730043/diff/1/setup.py#newcode10 setup.py:10: setup(name='trytond_gis', On 2017/03/17 14:36:10, ced wrote: > Maybe better: ...
7 months ago (2017-03-20 17:19:42 UTC) #3
nicoe
Fix comments and move regexp
7 months ago (2017-03-20 17:20:11 UTC) #4
nicoe
rename abstract_type and move regexp to prevent circular imports
7 months ago (2017-03-23 16:25:36 UTC) #5
nicoe
Add forgotten database.py
7 months ago (2017-03-23 16:41:51 UTC) #6
ced
https://codereview.appspot.com/322730043/diff/80001/setup.py File setup.py (right): https://codereview.appspot.com/322730043/diff/80001/setup.py#newcode16 setup.py:16: description='Adds GIS support to trytond', Maybe use: Geographical Information ...
5 months ago (2017-05-19 16:23:25 UTC) #7
ced
I'm wondering if we should not use https://github.com/cleder/pygeoif as suggested by https://stackoverflow.com/questions/14940285/using-postgis-on-python-3
5 months ago (2017-05-19 16:36:23 UTC) #8
nicoe
Use setUpModule to populate the module list and register the testing module
3 months, 4 weeks ago (2017-06-25 19:57:46 UTC) #9
nicoe
On 2017/05/19 16:36:23, ced wrote: > I'm wondering if we should not use https://github.com/cleder/pygeoif > ...
3 months, 4 weeks ago (2017-06-25 19:58:27 UTC) #10
nicoe
https://codereview.appspot.com/322730043/diff/80001/setup.py File setup.py (right): https://codereview.appspot.com/322730043/diff/80001/setup.py#newcode16 setup.py:16: description='Adds GIS support to trytond', On 2017/05/19 16:23:24, ced ...
3 months, 4 weeks ago (2017-06-25 19:58:37 UTC) #11
nicoe
create the postgis extension on the database creation
3 months, 3 weeks ago (2017-06-30 15:15:32 UTC) #12
nicoe
Use setuptools find_packages to find packages
3 months, 3 weeks ago (2017-06-30 16:40:26 UTC) #13
nicoe
Use geojson instead of dbgis
1 month, 1 week ago (2017-09-14 15:43:21 UTC) #14
nicoe
remove comment
1 month, 1 week ago (2017-09-14 16:53:56 UTC) #15
ced
https://codereview.appspot.com/322730043/diff/180001/README File README (right): https://codereview.appspot.com/322730043/diff/180001/README#newcode8 README:8: .. _flask_tryton bugtracker: http://trytond-gis.b2ck.com/ wrong name https://codereview.appspot.com/322730043/diff/180001/setup.py File setup.py ...
1 month ago (2017-09-15 09:49:21 UTC) #16
nicoe
Use geomet, add tests, move stuffs in cont.py and sql.py
1 month ago (2017-09-18 09:33:10 UTC) #17
nicoe
https://codereview.appspot.com/322730043/diff/180001/README File README (right): https://codereview.appspot.com/322730043/diff/180001/README#newcode8 README:8: .. _flask_tryton bugtracker: http://trytond-gis.b2ck.com/ On 2017/09/15 09:49:20, ced wrote: ...
1 month ago (2017-09-19 10:14:10 UTC) #18
ced
1 month ago (2017-09-20 17:28:21 UTC) #19
https://codereview.appspot.com/322730043/diff/180001/setup.py
File setup.py (right):

https://codereview.appspot.com/322730043/diff/180001/setup.py#newcode11
setup.py:11: setup(name='trytond_backend_gis',
On 2017/09/19 10:14:05, nicoe wrote:
> On 2017/09/15 09:49:20, ced wrote:
> > This name may be in conflict with the Foundation policy.
> > It should be prefixed by b2ck_ or we should propose it to host it on Tryton
> > project.
> 
> If it is to follow the tryton numbering scheme then we could propose it.

OK.

https://codereview.appspot.com/322730043/diff/200001/README
File README (right):

https://codereview.appspot.com/322730043/diff/200001/README#newcode7
README:7: 
If we do not have a doc folder, this should at least contain the list of new
fields available and how to configure the backend.

https://codereview.appspot.com/322730043/diff/200001/setup.py
File setup.py (right):

https://codereview.appspot.com/322730043/diff/200001/setup.py#newcode44
setup.py:44: # 'trytond>=4.6',
to be uncommented
also it should use the version of the package.

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/__init__.py
File trytond_gis/__init__.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/__init__.py#...
trytond_gis/__init__.py:9: class _GeoJSON(dict):
I do not find it is a good place to define such class.
It may generate difficulties later because all submodules need it.
I would put in a _data.py

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/__init__.py#...
trytond_gis/__init__.py:13: if 'meta' not in self:
why not use setdefault?

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/fields.py
File trytond_gis/fields.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/fields.py#ne...
trytond_gis/fields.py:1: # This file is part of trytond_gis.  The COPYRIGHT file
at the top level of
I find that the file should be:

trytond_gis/model/fields.py

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/fields.py#ne...
trytond_gis/fields.py:36: return _GeoJSON(value)
I think it should go thought the database sql_format

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/fields.py#ne...
trytond_gis/fields.py:51: expression = Operator(column,
self._domain_value(operator, value))
I think it will be better to have only one return:

if ...:
   Operator =
   value = null
else:
   Operator =
   value = self._domain_value()
return Operator(column, value)

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/fields.py#ne...
trytond_gis/fields.py:53: return expression
Why not return directly the Operator?

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/__in...
File trytond_gis/postgis/__init__.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/__in...
trytond_gis/postgis/__init__.py:1: from .table import *
Missing copyright header.

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/data...
File trytond_gis/postgis/database.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/data...
trytond_gis/postgis/database.py:1: # This file is part of trytond_backend_gis. 
The COPYRIGHT file at the top
I find that the file should be trytond_gis/backend/postgis/database.py

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/data...
trytond_gis/postgis/database.py:1: # This file is part of trytond_backend_gis. 
The COPYRIGHT file at the top
Wrong name

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/tabl...
File trytond_gis/postgis/table.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/postgis/tabl...
trytond_gis/postgis/table.py:1: # This file is part of trytond_backend_gis.  The
COPYRIGHT file at the top
wrong name

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/__init...
File trytond_gis/tests/__init__.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/__init...
trytond_gis/tests/__init__.py:1: from .test_gis import register
missing copyright header

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_f...
File trytond_gis/tests/test_fields.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_f...
trytond_gis/tests/test_fields.py:1: # This file is part of trytond_backend_gis. 
The COPYRIGHT file at the top
wrong name

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_f...
trytond_gis/tests/test_fields.py:19: "Testing create/write with GIS types"
It would be better to have a test for each one.

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_f...
trytond_gis/tests/test_fields.py:52: "Testing search with GIS types"
I think it is better to have one test per search.
What we have in trytond is no more maintainable so we should not replicate the
same mistake.

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_f...
trytond_gis/tests/test_fields.py:85: # For some reasons NULL are not taken into
account
It is not some reason, it is because NULL in any operation returns NULL.

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_g...
File trytond_gis/tests/test_gis/__init__.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_g...
trytond_gis/tests/test_gis/__init__.py:1: # This file is part of
trytond_backend_gis.  The COPYRIGHT file at the top
wrong name

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_g...
File trytond_gis/tests/test_gis/gis.py (right):

https://codereview.appspot.com/322730043/diff/200001/trytond_gis/tests/test_g...
trytond_gis/tests/test_gis/gis.py:1: # This file is part of trytond_backend_gis.
 The COPYRIGHT file at the top
wrong name
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted