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

feat: geodata reader #156

Merged
merged 5 commits into from
Jun 6, 2022
Merged

feat: geodata reader #156

merged 5 commits into from
Jun 6, 2022

Conversation

raphaelvignes
Copy link
Contributor

@raphaelvignes raphaelvignes commented Jun 2, 2022

Add a new geojson reader

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable

@raphaelvignes raphaelvignes self-assigned this Jun 2, 2022
@raphaelvignes raphaelvignes added the wip Work in progress label Jun 2, 2022
@raphaelvignes raphaelvignes force-pushed the feat/basemap-preview branch 2 times, most recently from e17b569 to 6a12fdd Compare June 3, 2022 09:59
@raphaelvignes raphaelvignes force-pushed the feat/basemap-preview branch from 6a12fdd to be6b0d6 Compare June 3, 2022 10:04
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #156 (bd79b00) into main (b502695) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #156   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        19    +1     
  Lines          796       806   +10     
=========================================
+ Hits           796       806   +10     
Impacted Files Coverage Δ
peakina/helpers.py 100.00% <100.00%> (ø)
peakina/readers/__init__.py 100.00% <100.00%> (ø)
peakina/readers/geodata.py 100.00% <100.00%> (ø)

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 b502695...bd79b00. Read the comment docs.

@raphaelvignes raphaelvignes removed the wip Work in progress label Jun 3, 2022
@@ -42,7 +43,7 @@ class TypeInfos(NamedTuple):


# For files without MIME types, we make fake MIME types based on detected extension
CUSTOM_MIMETYPES = {".parquet": "peakina/parquet"}
CUSTOM_MIMETYPES = {".parquet": "peakina/parquet", ".geojson": "application/geo+json"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we okay with enforcing this extension for basemaps ? cf. my comment here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is application/geo+json really a valid mimetype ? If not, I'd rather have peakina/geo to make it explicit that this is a custom mimetype. And I agree with fred's comment, the extension for geo files should probably be free (if that's possible in peakina)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if that's possible in peakina

MimeType guessing is not working it seems :/



@wraps(gpd.read_file)
def read_file(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename this function so it's explicit it's for geographic data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright 👍

Copy link
Contributor

@lukapeschke lukapeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM overall, but I have a few remarks

@@ -81,6 +86,7 @@ class TypeEnum(str, Enum):
JSON = "json"
PARQUET = "parquet"
XML = "xml"
GEOJSON = "geojson"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have GEOJSON here. I'd rather have something like GEODATA, and let it be a wildcard for everything that can be read by geopandas (even if we only support geojson in laputa for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

@@ -42,7 +43,7 @@ class TypeInfos(NamedTuple):


# For files without MIME types, we make fake MIME types based on detected extension
CUSTOM_MIMETYPES = {".parquet": "peakina/parquet"}
CUSTOM_MIMETYPES = {".parquet": "peakina/parquet", ".geojson": "application/geo+json"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is application/geo+json really a valid mimetype ? If not, I'd rather have peakina/geo to make it explicit that this is a custom mimetype. And I agree with fred's comment, the extension for geo files should probably be free (if that's possible in peakina)

@raphaelvignes raphaelvignes force-pushed the feat/basemap-preview branch from 3c33f84 to 70c5b7f Compare June 3, 2022 14:42
@raphaelvignes raphaelvignes force-pushed the feat/basemap-preview branch from 70c5b7f to bd79b00 Compare June 3, 2022 14:46
@raphaelvignes raphaelvignes changed the title feat: geojson reader feat: geodata reader Jun 6, 2022
Copy link

@jGundermann jGundermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@raphaelvignes raphaelvignes merged commit ca4b487 into main Jun 6, 2022
@raphaelvignes raphaelvignes deleted the feat/basemap-preview branch June 6, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants