search

sidewalklabs / s2sphere

Python implementation of the S2 geometry library.
http://s2sphere.sidewalklabs.com/
MIT License
213 stars 44 forks source link

linting with flake8 #8

Closed svenkreiss closed 8 years ago

svenkreiss commented 8 years ago

This change is Reviewable

danvk commented 8 years ago

Review status: 0 of 10 files reviewed at latest revision, 7 unresolved discussions.

_.travis.yml, line 56 [r1] (raw file):_ What is hacking and why do we need it? (I don't see it referenced anywhere.)

circle.yml, line 3 [r1] (raw file): Create a test.sh script? These lines are getting a little long.

Alternatively, you can create a YAML variable.

_setup.cfg, line 2 [r1] (raw file):_ Does this file support comments? If so, please document what this code is and why we want to ignore it.

s2sphere/init.py, line 2 [r1] (raw file): What is noqa? How about a comment.

s2sphere/sphere.py, line 14 [r1] (raw file): Is this coming directly from s2angle.h?

_tests/sphere_test.py, line 1508 [r1] (raw file):_ these don't fit on one line?

_tests/sphere_test.py, line 1812 [r1] (raw file):_ If this var is unused, it would be better to replace it with:

cell0tr.get_rect_bound()

so that the code still gets execute (it could still break the test if it threw an exception, for example).

Comments from Reviewable

danvk commented 8 years ago

Reviewed 10 of 10 files at r1. Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from Reviewable

svenkreiss commented 8 years ago

Review status: all files reviewed at latest revision, 7 unresolved discussions.

_.travis.yml, line 56 [r1] (raw file):_ It is a plugin to flake8. Adds more checks that I find pretty useful.

_circle.yml, line 3 [r1] (raw file):_ We are not at all using CircleCI as we are supposed to. Testing multiple versions in Python is much better supported in TravisCI. Was thinking of switching the purpose of Travis and Circle: test the C and Python compatibility on CircleCI and test multiple Python versions on TravisCI. But the original test of the C library was written for Travis :(

_setup.cfg, line 2 [r1] (raw file):_ Done.

s2sphere/init.py, line 2 [r1] (raw file): It's a flake8 thing. A comment that signals flake8 to ignore the file. I would also prefer if flake8 handled __init__.py files better, but this was the best I could find.

s2sphere/sphere.py, line 14 [r1] (raw file): It is from s1angle.h: https://github.com/micolous/s2-geometry-library/blob/master/geometry/s2/s1angle.h

Why?

_tests/sphere_test.py, line 1508 [r1] (raw file):_ 79 chars is pretty short

_tests/sphere_test.py, line 1812 [r1] (raw file):_ Done.

Comments from Reviewable

svenkreiss commented 8 years ago

Review status: 7 of 10 files reviewed at latest revision, 6 unresolved discussions.

_tests/sphere_test.py, line 1508 [r1] (raw file):_ it actually does, but the lines above dont and this keeps it symmetric

Comments from Reviewable

danvk commented 8 years ago
:lgtm:

Reviewed 3 of 3 files at r2. Review status: all files reviewed at latest revision, 3 unresolved discussions.

_circle.yml, line 3 [r1] (raw file):_ I think it would make sense to use either CircleCI or Travis, rather than both. With both, our tests will flake if either of them are having issues.

My suggestion is a little more concrete though—just to reduce code duplication by factoring out the python -V && pip install .[tests] && flake8 && nosetests -vv --exclude=compare_implementations_test" that's repeated 3x.

s2sphere/init.py, line 2 [r1] (raw file): Does the line have to be exactly this? Could it be

# flake8: noqa  (don't lint this file)

or

# flake8: noqa
# (don't lint this file)

s2sphere/sphere.py, line 14 [r1] (raw file): Just wondering about the rewording and whether it made our docstring more or less closely match the one in s1angle.h. I guess we will need to do some sort of manual QA on these, since comments about copy constructors don't make as much sense in Python.

Comments from Reviewable

svenkreiss commented 8 years ago

Review status: 8 of 10 files reviewed at latest revision, 2 unresolved discussions.

circle.yml, line 3 [r1] (raw file): never used a YAML variable. This seems to be a good solution. Testing it now.

s2sphere/init.py, line 2 [r1] (raw file): Done.

s2sphere/sphere.py, line 14 [r1] (raw file): our doc is not in great shape right now. The comments that are in are more or less in by accident. Was planning to do a proper pass and extract the C++ comments together with a refactor into R1, S1 and S2 sub-packages. Maybe that should be done before we talk about it publicly.

Comments from Reviewable

danvk commented 8 years ago

Reviewed 3 of 3 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable