jazzband / geojson

Python bindings and utilities for GeoJSON
https://pypi.python.org/pypi/geojson/
BSD 3-Clause "New" or "Revised" License
905 stars 120 forks source link

Add __getitem__ methods to sequence-like objects #103

Closed JesseWeinstein closed 6 years ago

JesseWeinstein commented 6 years ago

Makes accessing them easier. I wasn't sure if it was a good idea to make them writable or not.

frewsxcv commented 6 years ago

Thanks for opening the PR! I hate to close PR contributions, but I'm going to refer to my answer I gave in https://github.com/frewsxcv/python-geojson/issues/80. It seems simple enough just to access the coordinates property and then do the indexing. Also, if you have a Polygon(bbox = [...], coordinates=[...]), and you attempt to index, it's not super obvious if it would index coordinates or bbox. Sorry for closing, but thanks for the idea! 🙇

JesseWeinstein commented 6 years ago

Totally fine with you closing it, thank you for the prompt response.

What other properties can a Point have aside from coordinates? It looked from my reading of the spec that it (along with the other geometry objects) were required to only have type and coordinates (and bbox, but that does not appear to be supported by this library, unless I'm missing something.)

JesseWeinstein commented 6 years ago

@frewsxcv -- one last question. The previous discussion addressed making the coordinates property implicitly indexable, but this PR also does the same for the features and geometries properties. I do feel that the case for making those properties implicitly indexable is stronger, as their objects ( FeatureCollection and GeometryCollection) explicitly describe themselves as "Collections", and have no other functionality aside from containing those arrays.

Please reconsider accepting this more limited PR.

JesseWeinstein commented 6 years ago

Whoops, it looks like I need to make a new PR to show this. I'll wait to hear if you'd be interested -- but you can review the code here: https://github.com/frewsxcv/python-geojson/compare/master...JesseWeinstein:add_getitem

codecov-io commented 6 years ago

Codecov Report

Merging #103 into master will increase coverage by 0.21%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   92.51%   92.73%   +0.21%     
==========================================
  Files          11       11              
  Lines         334      344      +10     
  Branches       62       62              
==========================================
+ Hits          309      319      +10     
  Misses         16       16              
  Partials        9        9
Impacted Files Coverage Δ
geojson/feature.py 100% <100%> (ø) :arrow_up:
geojson/geometry.py 97.7% <100%> (+0.14%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1dd6e52...7a8a857. Read the comment docs.

frewsxcv commented 6 years ago

after giving it some more thought, i think this is change is okay. if it's possible, could you add a couple tests for this? if not, i can add some when i get some free time

frewsxcv commented 6 years ago

also fyi, added a merge commit to see how test coverage changes. feel free to get rid of the merge commit if you want

JesseWeinstein commented 6 years ago

Tests added, rebased off master. Merge away!

frewsxcv commented 6 years ago

thanks! bors r+

frewsxcv commented 6 years ago

bors r+

bors[bot] commented 6 years ago

Build succeeded