Modify

Opened 6 years ago

Closed 17 months ago

#17669 closed enhancement (fixed)

[PATCH][RFC] tag() in validator.mapcss should allow placeholders

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 23.06
Component: Core validator Version:
Keywords: template_report mapcss Cc:

Description

What steps will reproduce the problem?

*[/name/=~/.*([‘’]+).*/] {
  throwError: tr("Avoid using unicode '' in names (‘’) ({0})", "{0.key}");
  suggestAlternative: "'";
  fixAdd: concat("{0.key}", "=", tag("{0.key}"), " ", "{0.key}");
  assertNoMatch: "way name=\"Testing's Road\"";
  assertMatch: "way name=\"Testing‘s Road\"";
  assertMatch: "way name=\"Testing’s Road\"";
}

Test way:
way name=\"Testing‘s Road\"

What is the expected result?

I expect the name to become (with the above code) way name=\"Testing's Road name\".

What happens instead?

I get (with the above code) way name=\" name\".

Please provide any additional information below. Attach a screenshot if possible.

It does not appear to be a nested function issue (I checked with nested concats).

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-04-28 04:36:41 +0200 (Sun, 28 Apr 2019)
Revision:15031
Build-Date:2019-04-28 02:37:58
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (15031 en) Mac OS X 10.14.4
OS Build number: Mac OS X 10.14.4 (18E226)
Memory Usage: 375 MB / 2048 MB (116 MB allocated, but free)
Java version: 12+33, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: Display 188945226 1920x1080, Display 69978068 2048x1152
Maximum Screen Size: 2048x1152
Dataset consistency test: No problems found

Plugins:
+ Mapillary (1.5.18)
+ apache-commons (34908)
+ apache-http (34908)
+ auto_tools (73)
+ buildings_tools (34982)
+ continuosDownload (82)
+ graphview (34977)
+ gson (34908)
+ highwayNameModification (1555020960)
+ imagery_offset_db (34908)
+ jna (34908)
+ junctionchecking (34977)
+ kaartvalidation (1554390132)
+ mapdust (${version.entry.commit.revision})
+ markseen (13)
+ openqa (1556288504)
+ osm-obj-info (51)
+ reverter (34977)
+ rex (53)
+ terracer (34977)
+ todo (30306)
+ utilsplugin2 (34977)

Map paint styles:
+ ${HOME}/workspace/osm/JOSM Paint Styles and Presets/Kaart-Styles.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/DestinationSignRelation&zip=1
+ https://raw.githubusercontent.com/species/josm-preset-traffic_sign_direction/master/direction.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
+ https://raw.githubusercontent.com/OpenSidewalks/OpenSidewalks-Schema/master/open_sidewalks.mapcss

Validator rules:
+ ${HOME}/workspace/osm/kaart.validator.mapcss

Last errors/warnings:
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- W: Expecting test 'Avoid using unicode ' in names (‘’) ({0.key})' (i.e., [*[name'REGEX'.*([‘’']+).*]]) to not match way name="Testing's Road" (i.e., TagMap[name=Testing's Road])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5.0 (i.e., TagMap[highway=residential,maxweight=5.0])
- W: Expecting test '{0.key} is always in short tons (st)' (i.e., [*[maxweight][maxweight'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)], *[maxaxleload][maxaxleload'REGEX'^[0-9.]+$][ParameterFunction~inside(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String <US>)]]) to match way highway=residential maxweight=5 (i.e., TagMap[highway=residential,maxweight=5])
- E: Thread main-worker-0 raised java.lang.IllegalArgumentException: bytes must be >= 0

Attachments (1)

17669,22096.patch (45.7 KB ) - added by taylor.smock 18 months ago.
Add PlaceholderExpression for strings that need to have placeholders replaced

Download all attachments as: .zip

Change History (12)

comment:1 by Don-vip, 5 years ago

Component: Core mappaintCore validator
Keywords: template_report, mapcss → template_report mapcss

comment:2 by Don-vip, 5 years ago

Owner: changed from team to taylor.smock
Status: newneedinfo

Why don't you use "{0.tag}" instead of tag("{0.key}")?

comment:3 by taylor.smock, 5 years ago

I'm using a regex instead of a set value. {0.tag} gives name=.*([‘’]+).* and {0.value} gives .*([‘’]+).*. This indicated (to me) that the {0.tag}/{0.value} give the value as written in the test, not in the actual object. Since I'm not the only person writing validator.mapcss checks, I figured that changing the behavior of tag("{0.key}") would be the least likely to break other validator.mapcss checks publicly available (some might depend upon the current behavior of {0.tag}/{0.value}).

comment:4 by Don-vip, 5 years ago

Owner: changed from taylor.smock to team
Status: needinfonew

OK

comment:5 by simon04, 5 years ago

(Inventor of the nasty placeholders here.) A different approach would be to provide the actual value for {0.value} and {0.tag}.

comment:6 by taylor.smock, 5 years ago

I haven't looked at this in awhile, but I was worried (at the time), that some other third party validator.mapcss files would do something like this:

[name=~/^[A-Z] ((E|e)ast|(N|n)orth|(W|w)est|(S|s)outh)( [0-9/]+)? .*/],
[name=~/^[A-Z] [a-z]( [0-9/]+)? .*$/],
[name=~/^[A-Z][a-z]?( [0-9/]+)? .*$/] {
  throwError: tr("{0} matches the regex {1}. Check capitalization and spacing.", tag("name"), "{0.value}");
}

I haven't checked that for issues, so there might be syntax errors in there somewhere. (My county uses letters for road names, so E Road, ME Road, ME 3/4 Road, and so on, so I might end up using this to check roads in my local county sometime).

comment:7 by taylor.smock, 2 years ago

Possible duplicate: #22096

I haven't looked into this, but I rather suspect that fixing one will fix the other.

by taylor.smock, 18 months ago

Attachment: 17669,22096.patch added

Add PlaceholderExpression for strings that need to have placeholders replaced

in reply to:  5 comment:8 by taylor.smock, 18 months ago

Milestone: 23.06
Summary: tag() in validator.mapcss should allow placeholders[PATCH][RFC] tag() in validator.mapcss should allow placeholders

Notes on the patch:

  • Add selector field to Environment, uses record-style getter
  • Modifies MapCSSParser.jj to use PlaceholderExpression when the placeholder pattern matches. Calling ant javacc after applying this patch is required.
  • Where possible, set the selector for the Environment
  • Modify whichSelectorMatchesPrimitive(OsmPrimitive primitive) to create a new Environment with a MultiCascade -- this is required to have rules with !.something to not fail the autofix test.
  • Update expected toString output in MapCSSTagCheckerTest#testNaturalMarsh (the previous toString output used a string literal instead of an Expression)
  • Update numeric.mapcss to use the new functionality. This also changes some things around, partly for simplicity, and partly to better match the OSM unit standard.

EDIT: Additional locations in our source code that requires this functionality (using git grep '/\*.*{\d\+\.\(tag\|key\|value\)}.*\*/' -- resources):

Last edited 18 months ago by taylor.smock (previous) (diff)

comment:9 by taylor.smock, 18 months ago

Ticket #22096 has been marked as a duplicate of this ticket.

comment:10 by Famlam, 18 months ago

I can only review a little bit of it, but here's my feedback:

unusual value of {0}: use abbreviation for '' for foot and \" for inches, no spaces

Probably you mean:
unusual value of {0}: use '' for foot and \" for inches, no spaces
or: (but in this case the no spaces is confusing)
unusual value of {0}: use abbreviation or '' for foot and \" for inches, no spaces


set separator_autofix;

This separator_autofix isn't used elsewhere, probably it was intended to be on the rules that formerly had !.*_separator_autofix at the end of the selector? Otherwise you get a double warning (one for , instead of ., and one generic one). Probably a similar double warning occurs for a value which has a bad unit.

Last edited 18 months ago by Famlam (previous) (diff)

comment:11 by taylor.smock, 17 months ago

Resolution: fixed
Status: newclosed

In 18757/josm:

Fix #17669, #22096: Allow placeholders in more locations in MapCSS

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.