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

Fix GeoTrellisPath parsing #3191

Merged

Conversation

CloudNiner
Copy link
Contributor

@CloudNiner CloudNiner commented Feb 25, 2020

Overview

Parsing of url authority was bad. This should fix that. This PR also opened a discussion around whether absolute and relative urls without scheme should be valid as well so there's tests for those here that currently fail. If we want to support this, I can add a fix for that here as well, where scheme-less urls assume file://.

Before fix, 7 of new tests fail. After, only the scheme-less tests fail.

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • Module Hierarcy updated, if necessary
  • docs guides update, if necessary
  • New user API has useful Scaladoc strings
  • Unit tests added for bug-fix or new feature

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

LGTM; @CloudNiner you can also try to make schemelss URIs work and if none of the other tests would fail lets merge it 💯

@CloudNiner CloudNiner force-pushed the fixup/awf/geotrellis-path-parser branch from 9d6356d to d8764b0 Compare February 25, 2020 18:51
@CloudNiner CloudNiner changed the title [WIP] Fix GeoTrellisPath parsing Fix GeoTrellisPath parsing Feb 25, 2020
@CloudNiner
Copy link
Contributor Author

With fix applied:

scala> val rasterSource = RasterSource("s3://azavea-datahub/catalog?layer=nlcd-2011-epsg3857&zoom=13&band_count=1")
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
log4j:WARN No appenders could be found for logger (org.apache.http.client.protocol.RequestAddCookies).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
rasterSource: geotrellis.raster.RasterSource = GeoTrellisRasterSource(s3://azavea-datahub/catalog, Layer(name = "nlcd-2011-epsg3857", zoom = 13))
scala> rasterSource.extent
res3: geotrellis.vector.Extent = Extent(-1.4497467188222097E7, 2480629.988195896, -7087967.414370252, 6960317.577134008)

🎉

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

LGTM 👍 merging once travis is happy!

@CloudNiner CloudNiner merged commit 638ccf4 into locationtech:master Feb 25, 2020
@CloudNiner CloudNiner deleted the fixup/awf/geotrellis-path-parser branch February 25, 2020 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants