Merge geojsontables #42
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
if we want to treat it differently than the rest, it should not be part of the rest
* Switch to GitHub Actions for CI * don't test all point releases
to avoid having to define the geointerface over the generic JSON3.Object type
The geometry JSON objects generally only contain "type" and "coordinates". The type is already known since it is the name of the struct. Therefore we can just put the "coordinates" inside, which is a JSON3.Array. Doing some benchmarks on the previous geometry structs, which all needed to access the coordinates field, I found that simply accessing this field was quite slow. Implementing it this way avoids that cost. The downside is that "foreign members" that may be contained in the geometry object will be lost, however this is not required by the spec.
Just like JSON3.Array. This means a lot more methods will accept them.
From the geointerface get* functions. This ensures that the result of these functions also comply to the geointerface.
Requiring this led to quite the refactor. Now the geometry structs don't only store the `JSON3.Array` with their coordinates, but the entire object. This includes the optional bbox, and any GeoJSON object may have other members according to the spec. The object is not restricted to a `JSON3.Object`, since these are the products of reading JSON strings. If we want to create another sub-geometry using `GeoInterface.getgeom` we need to be able to create them as well, For this we use a NamedTuple in the geometry, similar to the `_lower` approach for writing.
The type parameter T is taken from peeking at the first feature if it exists. This is needed to make iterating over the features ini a featurecollection type stable. The features field was added after I noticed that the change above hardly helped iteration performance. On every `iterate` call the features had to be taken out of the JSON3.Object, which cannot be inferred. So it is better to keep them in the object, since they are a view on the same tape anyway.
- test GeoInterface.isclosed - add Aqua tests
Perhaps later on we can split tests into files such that this isn't all indented. But reading which tests fail is a pain without a root testset.
final push
rafaqz
force-pushed
the
merge_geojsontables
branch
from
95d0492
to
33f34d6
Compare
visr
approved these changes
Jun 30, 2022
There was a problem hiding this comment.
So if we merge this with a merge commit then the commits from both projects are retained in the history, right? Something like JuliaGraphs/Graphs.jl@9a25019
If so I'm fine with merging this. Several follow up commits are needed after that is done, ones I can think of now:
- rename GeoJSONTables to GeoJSON
- set the package UUID back to the GeoJSON one
- set the version number to be the next breaking release from v0.5 (perhaps let's jump to v1, so we can use semver fully?)
Since @yeesian started this package, I'd like his approval as well. For context see discussion in #36, and a list of known improvements in visr/GeoJSONTables.jl#15 (comment).
|
Haha I forgot to do the seach and replace on GeoJSONTables and fix the UUID. Will do now. |
This was referenced Jul 3, 2022
Closed
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR completely replaces GeoJSON.jl with the code form https://github.com/visr/GeoJSONTables.jl/
This gives compatibility with Tables.jl interface and GeoInterface.jl 1.0.
It also means lazing loading of objects with JSON3.jl.