Skip to content

add island labels (#399, #646, #703) - #737

Merged
zerebubuth merged 36 commits into
masterfrom
nvkelso/399-add-island-labels
Apr 28, 2016
Merged

zerebubuth merged 36 commits into
masterfrom
nvkelso/399-add-island-labels

Conversation

@nvkelso

@nvkelso nvkelso commented Apr 21, 2016

Copy link
Copy Markdown
Member

Connects with #399 to add island, islet & other labels. Also refactors how the earth layer is constructed.

  • [query] add earth queary JINJA
  • [query] hook up schema prefunction for new earth layer
  • [query] hook up create sql functions and sql.jinja to new earth layer
  • [filter] remove continent place labels from places layer
  • [filter] init earth YAML filter, add continent place label to earth layer
  • [filter] adjust island, islet zoom ranges
  • [apply] hook up data/apply functions, set new fields for new earth layer
  • [tests] add full set of island label tests
  • [migration] add custom migration for new earth layer features
  • [docs] add new kinds to earth layer docs

@nvkelso nvkelso added this to the v0.10.0 milestone Apr 21, 2016
@nvkelso

nvkelso commented Apr 21, 2016

Copy link
Copy Markdown
Member Author

@zerebubuth This is still a WIP, but it's pretty close to done. Please have a look around, particularly at migration and query parts.

I've verified the zooms are working well in the DB for the test cases, but still need to fully run the test via python.

@nvkelso

nvkelso commented Apr 21, 2016

Copy link
Copy Markdown
Member Author

Also connects to #703 to move the continent label placement from places layer to earth layer.

@nvkelso

nvkelso commented Apr 21, 2016

Copy link
Copy Markdown
Member Author

Also connects to #646 to move the last layer, earth, to the new YAML setup.

Comment thread queries/earth.jinja2 Outdated
max(gid) AS __id__
max(gid) AS __id__,
NULL AS label_placement,
NULL AS boundary

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.

I think we prefer to do the addition of extra parameters (label_placement, boundary) via YAML now, to avoid having to do NULL AS ... in the other queries.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, I'll revise.

(This is loosely based off the water.jinja2 file, probably similar cleanup over there, but I'd rather not mess with that in this PR.)

@nvkelso
nvkelso force-pushed the nvkelso/399-add-island-labels branch from d8cba1a to 5c11337 Compare April 25, 2016 01:27
@nvkelso
nvkelso force-pushed the nvkelso/399-add-island-labels branch from c698f2e to 6a01957 Compare April 25, 2016 12:22
@nvkelso

nvkelso commented Apr 25, 2016

Copy link
Copy Markdown
Member Author

I've got it all working (!?) except for these handful of tests which fail:

Similar test do pass, though, and the features exist in the DB and seem to be marked with the expected zoom values.

The synthetic_columns: - test bit should also be investigated & resolved.

Because I'll be traveling the rest of the week, please pick up this PR and see it to completion. Thanks!

@nvkelso nvkelso changed the title add island labels (#399) add island labels (#399, #646, #703) Apr 25, 2016
@zerebubuth

Copy link
Copy Markdown
Member

Tests pass for me. I think all the issues above should be fixed now. @rmarianski could you review this, please?

@rmarianski

Copy link
Copy Markdown
Member

👍

@zerebubuth
zerebubuth merged commit 7718f17 into master Apr 28, 2016
@zerebubuth
zerebubuth deleted the nvkelso/399-add-island-labels branch April 28, 2016 08:45
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.

3 participants