Multiple input sources #2
Conversation
concrete classes for GeoJSON and Shapefiles.
|
I wouldn't bother explicitly labelling the superclass as abstract, since you might find later you want to have a geopandas-specific container (as opposed to general dataframe) and this would be the obvious place to do it. |
|
I'm not sure I understand @martindurant, can you elaborate? My thought here was that these |
|
They are indeed two separate issues, and you are right in your understanding - I was just thinking that the superclass could also be the container class. Or maybe not - just a suggestion. I just personally never actually label a class explicitly as abstract, but maybe that's just my style. |
|
Got it, thanks. I'm spending some significant time in Python right now after a long-ish time in Javascript, so I'm still playing around with style choices : ) |
|
ping @jacobtomlinson any thoughts here? |
There was a problem hiding this comment.
Apologies, I'd left it as the title still says WIP.
This looks awesome! Thanks for taking the time to do this.
The only issue I've seen is the missing name for the GeoPandasSQLSource.
There are a few lint issues that have been introduced, but I doubt it was correct before anyway. Feel free to optionally correct those.
| class ShapefileSource(GeoPandasFileSource): | ||
| name="shapefile" | ||
|
|
||
| class GeoPandasSQLSource(GeoPandasSource): |
There was a problem hiding this comment.
Missing name attribute.
There was a problem hiding this comment.
The intention here was that this not actually be instantiated, and that users either select a postgis or sptialite source. It may end up being a distinction without a difference, but I kind of liked being more explicit about the source type. What do you think?
There was a problem hiding this comment.
You may want to make sure that the correct classes and names end up in intake.registry. That could be as simple as not exposing classes you don't intend for end-users in the top level of the package.
There was a problem hiding this comment.
It seems to be discovered by the registry as intended. The GeoPandasSQLSource is not exported in the __init__.py above.
|
Thanks for taking a look @jacobtomlinson! I still have it marked as WIP as I had hoped to get some tests working for Spatialite at least. I've had some trouble getting that working in CI, though. |
ian-r-rose
force-pushed
the
multiple-input-sources
branch
from
d715c3c
to
7a69a25
Compare
ian-r-rose
force-pushed
the
multiple-input-sources
branch
from
7a69a25
to
565f932
Compare
|
Okay, this is ready for another look. I've added a couple of SpatiaLite tests that are based off of ones in GeoPandas. Unfortunately, they rely on bugfixes that are not in a published version, but I have verified that they pass locally using geopandas master, and marked them as xfail for now. |
|
Released in |
|
Thanks for the review and quick publish! |
|
Does this mean that intake-dcat can now be released? |
|
I am waiting on dask/dask#4634, which is required for it to be able to access datasets from Socrata sites. Then I plan to publish a quick release to PyPI. It would be useful if we could also publish |
|
Yes, please, to all of that! |
Fixes #1.
Still need to add a version for PostGIS and write a bunch of tests, but I wanted to get your feedback sooner rather than later @jacobtomlinson. What do you think?