Skip to content

Pass variables into grid_renderer#769

Open
rochoa wants to merge 1 commit into
mapnik:masterfrom
CartoDB:grid-variables
Open

Pass variables into grid_renderer#769
rochoa wants to merge 1 commit into
mapnik:masterfrom
CartoDB:grid-variables

Conversation

@rochoa

@rochoa rochoa commented May 16, 2017

Copy link
Copy Markdown

Use a similar approach as in EIO_RenderImage, making variables available at renderer/datasource level.

It should fix #768.

Use a similar approach as in EIO_RenderImage, making variables
available at renderer/datasource level.

It should fix mapnik#768.
@flippmoke

Copy link
Copy Markdown
Member

Can you please add test cases for this?

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #769 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
+ Coverage   96.23%   96.23%   +<.01%     
==========================================
  Files          42       42              
  Lines        8810     8813       +3     
==========================================
+ Hits         8478     8481       +3     
  Misses        332      332
Impacted Files Coverage Δ
src/mapnik_map.cpp 99.92% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c335992...0f64ee5. Read the comment docs.

@rochoa

rochoa commented May 17, 2017

Copy link
Copy Markdown
Author

@flippmoke, I will be more than happy to add some tests for this one 👍 .

However, I find it a bit difficult as there are no tests for the Postgis input plugin and I don't know about other plugins (I checked shape, geojson, and memory ones) supporting the variables functionality.

How should I proceed? Should I change the CI scripts to install PosgreSQL/Postgis to test it?

Thanks 😄 .

@rochoa

rochoa commented May 22, 2017

Copy link
Copy Markdown
Author

@flippmoke, I'm really willing to add the test for this one. How should I proceed?

@flippmoke

Copy link
Copy Markdown
Member

I believe the render tests for grid renderer are located here -- https://github.com/mapnik/node-mapnik/blob/master/test/grid.test.js

It looks like the vector tile renderer already supports this --- https://github.com/mapnik/node-mapnik/blob/master/src/mapnik_vector_tile.cpp#L5174.

So it has some tests here:

https://github.com/mapnik/node-mapnik/blob/master/test/vector-tile.test.js#L2664

Those are probably your best examples.

Could we do this with out the postgis plugin but using another plugin?

@lightmare

Copy link
Copy Markdown

Could we do this with out the postgis plugin but using another plugin?

I think only postgis and pgraster plugins support variables atm.

@rafatower

Copy link
Copy Markdown
Contributor

This is a fairly small and safe change that we integrated moths ago into our fork and has been running in our production premises ever since.

Anyway, now we can take advantage of the support for postgis tests added here (thanks!): #809

I'll take a look whenever I have a chance to write a few tests for it...

@springmeyer

Copy link
Copy Markdown
Member

👍 @rafatower after #884 we're all 🍏 as far as tests. Please create a new PR (or I guess add to this one) when you have tests in place and then I'll merge.

@springmeyer springmeyer modified the milestones: v4.0.0, 4.0.1 Jun 25, 2018
@artemp artemp removed this from the 4.0.1 milestone Nov 22, 2024
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.

Missing options.variables for Grids

7 participants