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

Basic support to geojson #153

Closed
wants to merge 7 commits into from
Closed

Basic support to geojson #153

wants to merge 7 commits into from

Conversation

gperonato
Copy link
Contributor

Feature collection of points

@gperonato gperonato mentioned this pull request Nov 21, 2020
Copy link
Member

@akariv akariv left a comment

Choose a reason for hiding this comment

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

Hey @gperonato - thanks for the PR!! I left a few comments and questions.

dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
@akariv
Copy link
Member

akariv commented Nov 24, 2020

@gperonato lmk when you're ready for another review

@gperonato
Copy link
Contributor Author

Ready, sorry for the multiple commits!

@akariv
Copy link
Member

akariv commented Nov 24, 2020

Hey, so a few things (apart from the CR fixes):

  • Please include in your PR also the necessary modifications to documentation
  • Please add a test for this new functionality
  • Please rebase the branch to ensure you're based on the latest master branch

Copy link
Member

@akariv akariv left a comment

Choose a reason for hiding this comment

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

Some simplification and clarification comments

dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
dataflows/processors/dumpers/file_formats.py Outdated Show resolved Hide resolved
@akariv
Copy link
Member

akariv commented Dec 2, 2020

@gperonato lmk if you need any help in finalizing this... it's really close to the finish line :)

@gperonato
Copy link
Contributor Author

Can you check the latest commit?

@n0rdlicht
Copy link

Super cool you have been working on this @gperonato 👍 . If we (also my colleagues at cividi, e.g. @loleg or @bkarolina) can help getting this done in any way, let us know.

@gperonato
Copy link
Contributor Author

Super cool you have been working on this @gperonato 👍 . If we (also my colleagues at cividi, e.g. @loleg or @bkarolina) can help getting this done in any way, let us know.

Thank you, great to see there is some interest in this feature! Let's coordinate on #152, I think accepting a generic GeoJSON would be a nice add on, I haven't done any test on that

@akariv
Copy link
Member

akariv commented Dec 22, 2020

Thanks @gperonato!

I rebased the code to the latest master and did some cleanup, you can check it out in #155

@akariv akariv closed this Dec 22, 2020
@akariv akariv reopened this Dec 22, 2020
@loleg
Copy link

loleg commented Dec 22, 2020

Many thanks for this highly useful effort. A couple of thoughts/questions that could be converted to tickets from my side:

  • GeoJSON is a subset of / extends JSON support, but by having to explicitly initialise it (rather than load as a separate module, let's say) it seems to be treated on par. Would we do the same for TSV? SQLite? GeoPackage? But I guess it's too early to worry about scalability when there are only 3 formats supported at this time.
  • On this note, the introduction of default_serializer to file_formats.py might warrant it's own PR, or at least a check through the other code and documentation.
  • What about auto-detecting a GeoJSON file?
  • We should link to RFC 7946 or similar. To me this effort is also closely related to the effort to create a Geo Data Package. Currently the README doesn't really cover file format support, but it should.
  • We might want to stick to a .geojson file extension in the example, though make it clear that this is not required.
  • Where does validation happen? I.e. where would you hand off to a more specialized library like python-geojson.
  • What about streaming support, e.g. would dataflows handle use cases similar to https://github.com/4xle/geojson-vt-stream ?
  • It would be very helpful to add a section to the Tutorial

@akariv
Copy link
Member

akariv commented Sep 24, 2021

Closing in favor of #174

@akariv akariv closed this Sep 24, 2021
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