Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Validation missing for selectors not matching layers #29

Closed
springmeyer opened this issue Apr 29, 2011 · 8 comments
Closed

Validation missing for selectors not matching layers #29

springmeyer opened this issue Apr 29, 2011 · 8 comments
Milestone

Comments

@springmeyer
Copy link

This works without warning or error if #foo is not a valid layer.

#foo {
  bar:bar
}
@tmcw
Copy link
Contributor

tmcw commented Apr 29, 2011

Hmm. I think this should only do anything if we can institute warnings - errors would be too harsh a judgment for something that's perfectly validating carto & potentially useful (for previewing maps, excluding layers temporarily)

@springmeyer
Copy link
Author

yep, warnings would be great, I agree an error would be too harsh.

@tokumine
Copy link

Hi there, just a quick report as I came across this today.

what triggers
A property of a non-matching selector was misspelled.

expected
Errors for this misspelling would still be given as the property was not valid carto.

observed
As the validation was not fired for this selector no errors were thrown.

On warnings vs errors, I wonder if errors are a more predictable response here. It wouldn't stop code being temporarily disabled via comments for example.

@tmcw
Copy link
Contributor

tmcw commented Aug 11, 2011

Errors for this misspelling would still be given as the property was not valid carto.

So far this isn't true: Carto stylesheets are valid in grammar and properties but are not validated as in 'this stylesheet will validate this data'. Making that determination means that right now there's virtually no talking between Mapnik and Carto, and no need to load datasources in validation.

Of course, this needs to change.

@tokumine
Copy link

So far this isn't true: Carto stylesheets are valid in grammar and properties but are not validated as in 'this stylesheet will validate this data'.

I think there may be a misunderstanding (probably on my part).

In my case the carto stylesheet was actually not valid in properties (there was a misspelt property) yet was passed as valid because the unmatched selector it was under does not seem to be included in the syntax validation step.

Is it expected behaviour that if the selector doesn't have a corresponding Layer in the MML it's grammar/syntax is not validated?

I hear what you're saying about stylesheets validating data in Mapnik - I'm really looking forwards to seeing how far carto gets integrated into Mapnik with the GSOC work.

edit: code is better. This is a example MML that doesn't report any errors on render:

{
    "srs": "+proj=merc...",
    "Stylesheet": [
        {'id': "style.mss", 'data':' #non-existant-layer{bad-tag:bad_value;}'} 
    ],
    "Layer": [{
        "name": "world",...
    }]
}

@springmeyer springmeyer removed this from the 0.9.6 milestone Jul 9, 2015
@nebulon42 nebulon42 added this to the 0.18 milestone Jan 20, 2017
@nebulon42 nebulon42 modified the milestones: 0.18, 1.0 May 11, 2017
nebulon42 added a commit that referenced this issue May 28, 2017
…rogram failure, warn when layer has no associated styles, warn when styles do not match correspondig layer selector, ref #29
@nebulon42
Copy link
Collaborator

We have now warnings when a layer has no associated styles and when styles do not match a corresponding layer selector.

The following situation (taken from osm-carto):

#admin-low-zoom[zoom < 11],
#admin-mid-zoom[zoom >= 11][zoom < 13],
#admin-high-zoom[zoom >= 13] {
  [admin_level = '4'] {
    [zoom >= 12] {
      background/line-width: 3;
      line-width: 3;
    }
  }
}

generates a warning about styles not matching the layer selector #admin-low-zoom. This is because #admin-low-zoom is filtered only for zooms < 11. The warning is correct but this code is also legit. We should figure out a way if we could suppress this kind of warnings because they are likely to create some noise.

@nebulon42
Copy link
Collaborator

nebulon42 commented May 28, 2017

As for the other error report: Currently rules are only validated upon serializing them to XML. I'm not sure if it is worth to change that.

Edit: the added complexity is not worth it.

nebulon42 added a commit that referenced this issue May 28, 2017
@nebulon42
Copy link
Collaborator

I think it does not make sense to delay 1.0 any longer because of #29 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants