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

Pass variables into grid_renderer #769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rochoa
Copy link

@rochoa rochoa commented May 16, 2017

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
Member

Can you please add test cases for this?

@codecov-io
Copy link

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
Copy link
Author

rochoa commented May 17, 2017

@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
Copy link
Author

rochoa commented May 22, 2017

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

@flippmoke
Copy link
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

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
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
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