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

Fix TemporalProjectedExtentCodec to handling proj4 strings when CRS isn't available #2034

Merged
merged 4 commits into from Mar 9, 2017

Conversation

Projects
None yet
4 participants
@metasim
Member

metasim commented Feb 28, 2017

Fixes #2033

@lossyrob

This comment has been minimized.

Member

lossyrob commented Feb 28, 2017

I'm +1 on this, @echeipesh your review was requested.

}
else {
rec.put("proj4", crs.toProj4String)
}

This comment has been minimized.

@metasim

metasim Mar 1, 2017

Member

Another (breaking) option would be to replace epsg all together with crs, and prefix EPSG codes according to this, falling back to "proj4" strings otherwise.

This comment has been minimized.

@lossyrob

lossyrob Mar 1, 2017

Member

With Avro schema migration capabilities (which allow for aliasing), this might not have to be a breaking change. See: https://github.com/locationtech/geotrellis/blob/master/spark/src/main/scala/geotrellis/spark/io/avro/codecs/KeyCodecs.scala#L52

@metasim

This comment has been minimized.

Member

metasim commented Mar 6, 2017

@pomadchin Good catch. Will look for a mechanism to share code (independent codec?).

@echeipesh are you good with this?

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Mar 7, 2017

+1, nice that it was kept from being a breaking change. Shame that ProjectedExtentCodec can't directly use the CRS codec.

@metasim

This comment has been minimized.

Member

metasim commented Mar 7, 2017

@echeipesh Created a shared CRS codec.

@metasim

This comment has been minimized.

Member

metasim commented Mar 7, 2017

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Mar 9, 2017

@lossyrob As I'm reading this Avro schemes they fall under schema migration, so that isn't breaking: old records can be read using new versions. The new CRSCodec class is new so since we're adopting SemVer would place this change into 1.1.0, which seems alright to me.

Probably the moral is that we'll have to release 1.1.0 soon after 1.0.1 as a lot of the PRs that have been merged in since 1.0.0 have added classes or methods.

@echeipesh echeipesh added this to the 1.1 milestone Mar 9, 2017

@echeipesh echeipesh merged commit 94dce0c into locationtech:master Mar 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lossyrob lossyrob modified the milestones: 1.0.1, 1.1 Mar 10, 2017

@metasim metasim deleted the metasim:fix/2033 branch Mar 10, 2017

@lossyrob

This comment has been minimized.

Member

lossyrob commented Mar 12, 2017

@metasim for future reference: The PR should be named describing the change, and the body of the PR should say "Fixes #XXXX" - this way they are properly linked in GitHub, and the issue actually closes once the PR is merged. This way (with Fixes comment in a commit, and issue number in PR name) took just a bit to track down, no big deal but just a heads up :)

I made changes to the PR title and description

@lossyrob lossyrob changed the title from Fix/2033 to Fix TemporalProjectedExtentCodec to handling proj4 strings when CRS isn't available Mar 12, 2017

@lossyrob lossyrob modified the milestones: 1.1, 1.0.1 Mar 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment