Skip to content

Fix GeoTrellisPath parsing - #3191

Merged
CloudNiner merged 4 commits into
locationtech:masterfrom
CloudNiner:fixup/awf/geotrellis-path-parser
Feb 25, 2020
Merged

Fix GeoTrellisPath parsing#3191
CloudNiner merged 4 commits into
locationtech:masterfrom
CloudNiner:fixup/awf/geotrellis-path-parser

Conversation

@CloudNiner

@CloudNiner CloudNiner commented Feb 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

pomadchin
pomadchin approved these changes Feb 25, 2020
View reviewed changes

@pomadchin pomadchin left a comment

Copy link
Copy Markdown
Member

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 💯

Comment thread store/src/test/scala/geotrellis/store/GeoTrellisPathSpec.scala Outdated
@CloudNiner CloudNiner mentioned this pull request Feb 25, 2020
Closed
@pomadchin pomadchin assigned CloudNiner and unassigned pomadchin Feb 25, 2020
@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

CloudNiner commented Feb 25, 2020

Copy link
Copy Markdown
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)

🎉

pomadchin
pomadchin approved these changes Feb 25, 2020
View reviewed changes

@pomadchin pomadchin left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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
@CloudNiner CloudNiner mentioned this pull request Feb 26, 2020
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pomadchin pomadchin pomadchin approved these changes

Assignees

@CloudNiner CloudNiner

Labels

bug unplanned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@CloudNiner @pomadchin