MapCSS-based tag checker/fixer

[simon04]

I'm considering to implement a tag checker/fixer based on the MapCSS syntax which subsumes the functionality of TagChecker and DeprecatedTags.

For instance, the line

way  : I : highway == footway && maxspeed == *                 # maxspeed used for footway

from tagchecker.cfg could be written as

way[highway=footway][maxspeed] {
  throwInformation: 'maxspeed used for footway';
}

And a test from DeprecatedTags

        checks.add(new DeprecationCheck(2110).
                testAndRemove("natural", "marsh").
                add("natural", "wetland").
                add("wetland", "marsh"));

could look like

*[natural=marsh] {
   throwWarning: 'natural=marsh is deprecated';
   fixRemove: 'natural=marsh';
   fixAdd: 'natural=wetland';
   fixAdd: 'wetland=marsh';
}

An implementation of this seems very easy, since we can use MapCSSParser. This would get rid of some custom file formats plus parsers and also allows end users to write custom tests/fixes.

What do you think?

[Don-vip]

A big +1 !!!!

I was thinking about rewriting tagchecker.cfg as the syntax is very poor, I was just not sure about how to start.

Just one note concerning the deprecation message; it would be better to not have to translate several times "xxxx is deprecated" :)

[Don-vip]

Also, it would be great to make some cleanup in data folder.

I always find hard to locate the good config file, they are almost hidden between all the .lang files.

Subfolders like data/i18n, data/validator would be great :)

[skyper]

Replying to Don-vip:

A big +1 !!!!

+1

Just one note concerning the deprecation message; it would be better to not have to translate several times "xxxx is deprecated" :)

+1

Replying to Don-vip:

Also, it would be great to make some cleanup in data folder.

I always find hard to locate the good config file, they are almost hidden between all the .lang files.

Subfolders like data/i18n, data/validator would be great :)

Yes, this is a mess.

[simon04]

:-)

MapCSS supports functions, so let's directly call tr() to avoid many similar translations:

*[natural=marsh] {
   throwWarning: tr("{0} is deprecated", "natural=marsh");
   fixRemove: "natural";
   fixAdd: "natural=wetland";
   fixAdd: "wetland=marsh";
}

The implementation is quite complete, some testing is still required. Then, TagChecker and DeprecatedTags have to be migrated …

[simon04]

Btw: the currently supported format is:

* {
   /* exactly one of */
   throwError: /*...*/
   throwWarning: /*...*/
   throwOther: /*...*/

   /* arbitrarily many of */
   fixAdd: "key=val";
   fixRemove: "key";
   fixChangeKey: "old=>new";
   suggestAlternative: "key";
   suggestAlternative: "key=val";
}

This should allow to supersede TagChecker and DeprecatedTags. Did I miss something?

[Don-vip]

#9409 ? Can we implement it at the same time ?

[simon04]

Replying to Don-vip:

#9409 ? Can we implement it at the same time ?

The easiest way might be to extend the MapCSSImplementation to have all condition selectors also in a variant to compare the values of two specified keys. The challenge would be to find a nice syntax? What about an analogy to the de-reference operator, e.g., way[name=*wikipedia] {...}?

[Don-vip]

Mmmm I'm not a syntax expert, let me call a friend :)

[simon04]

In 6506/josm:

see #9414 - implement MapCSS-based tag checker/fixer

The file deprecated.mapcss contains all DeprecatedTags tests. The
latter is to be removed eventually.

[simon04]

[o30144] - JOSM: include validator/*mapcss in i18n process

[simon04]

To be considered: #8687 - include positive+negative examples to validation tests. Possible syntax: assertMatch: "way maxwidth=2,5" and assertNoMatch: "maxwidth=2.5".

[simon04]

In 6511/josm:

see #9414 - MapCSSTagChecker: handle tagging alternatives

[simon04]

In 6512/josm:

see #9414 - MapCSSTagChecker: parse and unit test match assertions (assertMatch, assertNoMatch)

[simon04]

In 6513/josm:

see #9414 - convert some tagchecker tests to MapCSS, extend MapCSS by test for false values

[Don-vip]

In 6529/josm:

Various stuff:

• see #9414: remove old DeprecatedTags test
• refactor some classes in gui.preferences package
• improve javadoc

[Don-vip]

In 6532/josm:

fix #8687, see #9414, see #9470 - tagchecker: update numeric tests to new MapCSS format, with embedded unit tests. MapCSS syntax updated a bit for regex.

[simon04]

In 6534/josm:

fix #9470 see #9414 - make "layer tag with + sign" auto fixable

[simon04]

In 6537/josm:

see #9414 - migrate relation and religion tagchecks to MapCSS, add more "X relation without tag X" checks

[simon04]

In 6538/josm:

see #9414 - MapCSS-based tagchecker: provide {i.key}, {i.value}, {i.tag} variables to messages/fixes which refer to the corresponding match condition

[Don-vip]

Replying to simon04:

provide {i.key}, {i.value}, {i.tag} variables to messages/fixes which refer to the corresponding match condition

Waw, this is really great !

[simon04]

In 6548/josm:

see #9414 - MapCSS-based tagchecker: migrate remaining tagchecks

All former tagchecker.cfg checks are now located in data/validator/*mapcss :-)

[Don-vip]

Well done ! :)

[simon04]

In 6550/josm:

see #9414 - MapCSS-based tagchecker: unify some checks

[simon04]

In 6553/josm:

see #9414 - MapCSS-based tagchecker: allow to add custom files in preferences (resumes r6551)

[simon04]

In 6554/josm:

see #9414 fix #9409 - extend MapCSS condition syntax to allow the comparison of two key values

The syntax is [key1 = *key2] where * is inspired by the C de-reference operator, and = stands for any of =/!=/~=/^=/$=/*=/=~/!~.

[simon04]

Started the specification in the Wiki: Help/Validator/MapCSSTagChecker

[simon04]

In 6601/josm:

see #9414 fix #9485 - MapCSSTagChecker: add support for set class_name instruction and .class_name condition

[Don-vip]

this is one of the greatest improvements ever made to the validator since ages :)

[simon04]

In 6636/josm:

see #9414 - MapCSS validator: skip tests with informational severity if that isn't enabled

[simon04]

In 6637/josm:

see #9414 - Validator: declassify "street name contains ss" to informational level (as it was in tagchecker's times)

[stoecker]

Hmm, there is a difference between Upload and Non-Upload in severity handling. It seems you use the normal setting also for uploads in r6636.

[stoecker]

Some notes:

• Be careful with i18n - It is fine to add non-translatable English text, like tags. It is not ok to insert already translatet texts in a tr() argument - that breaks most slavic languages!
• Why is there a difference between the mapcss tests and the other validator sources. Can't the be handled in one config entry?
• We need to get the ignore feature back.
• Severity handling for upload is not yet correct (see previous comment)
• If you deprecated them, maybe we should drop all the other files and only use one format.
• If we allow user written validation checks, then be prepared, that they are made like styles and presets and also stored in zip files. The extension .mapcss is thus a bad choice, as it will conflict with styles
  • presets and styles can be in one zip ATM, even if this is used rarely
  • For validator checks I assume this is more likely to happen

[simon04]

Replying to stoecker:

Some notes:

• Be careful with i18n - It is fine to add non-translatable English text, like tags. It is not ok to insert already translatet texts in a tr() argument - that breaks most slavic languages!

Okay. Only throwWarning: tr("{0} used with {1}", tr("major road"), "{0.tag}"); should be affected.

• Why is there a difference between the mapcss tests and the other validator sources. Can't the be handled in one config entry?

I tried to split them up a bit by category. No need for that, though. Do you prefer a single core.validator.mapcss file?

• We need to get the ignore feature back.

That is, setting the description_en test error field?

• Severity handling for upload is not yet correct (see previous comment)

Which part is incorrect? r6636 basically implements a pre-check to skip tests which generate informational warnings when they are not going to be displayed. A real filtering is still applied later on. This is for performance reasons (to skip the "Crossing Areas" test mostly).

• If you deprecated them, maybe we should drop all the other files and only use one format.

I stepped back from removing the .cfg tagchecker since mappers may have defined such files for their personal use. I thought it might be better to keep the code for now to not provoke them. :-)

• If we allow user written validation checks, then be prepared, that they are made like styles and presets and also stored in zip files. The extension .mapcss is thus a bad choice, as it will conflict with styles
  • presets and styles can be in one zip ATM, even if this is used rarely
  • For validator checks I assume this is more likely to happen

Good point to consider. For instance, we could use *.validator.mapcss (like for *.user.js user scripts in the browser). This does not break available syntax highlighting etc.

[stoecker]

• Be careful with i18n - It is fine to add non-translatable English text, like tags. It is not ok to insert already translatet texts in a tr() argument - that breaks most slavic languages!

Okay. Only throwWarning: tr("{0} used with {1}", tr("major road"), "{0.tag}"); should be affected.

I already fixed two others :-)

• Why is there a difference between the mapcss tests and the other validator sources. Can't the be handled in one config entry?

I tried to split them up a bit by category. No need for that, though. Do you prefer a single core.validator.mapcss file?

No I meant why are there two sections in validator config to add additional files. Anyway if we drop the others, this problem is academic.

• We need to get the ignore feature back.

That is, setting the description_en test error field?

I think so. We either can make a hash from this and keep the number based approach or directly use the text. Thought the text must be without argument expansion usually, otherwise you can't ignore error groups.

Needs a rework of all the internal tests as well. Phew.

• Severity handling for upload is not yet correct (see previous comment)

Which part is incorrect? r6636 basically implements a pre-check to skip tests which generate informational warnings when they are not going to be displayed. A real filtering is still applied later on. This is for performance reasons (to skip the "Crossing Areas" test mostly).

Ah, but still, why don't you use the relevant config upload/non-upload?

• If you deprecated them, maybe we should drop all the other files and only use one format.

I stepped back from removing the .cfg tagchecker since mappers may have defined such files for their personal use. I thought it might be better to keep the code for now to not provoke them. :-)

You probably can count users with own validator configs on one hand. Remove old stuff when no longer needed. This is a major improvement, so we can break some small details. Also most of good/bad wordlist probably can be dropped as well. Haven't seen such messages for a long time.

• If we allow user written validation checks, then be prepared, that they are made like styles and presets and also stored in zip files. The extension .mapcss is thus a bad choice, as it will conflict with styles
  • presets and styles can be in one zip ATM, even if this is used rarely
  • For validator checks I assume this is more likely to happen

Good point to consider. For instance, we could use *.validator.mapcss (like for *.user.js user scripts in the browser). This does not break available syntax highlighting etc.

Fine.

[simon04]

In 6645/josm:

see #9414 - MapCSS validator: some performance imrovements (pre-compile regular expressions, drop regular expressions for key presence checks)

[simon04]

In 6649/josm:

see #9414 - MapCSS validator: make individual tests ignorable

As "ignore"-key, the MapCSS selector is used, e.g., *[name'REGEX'(?i).*Strasse.*].
The file validator/ignorederrors then contains entries like 3000_*[name'REGEX'(?i).*Strasse.*].

[simon04]

Replying to stoecker:

• Why is there a difference between the mapcss tests and the other validator sources. Can't the be handled in one config entry?

I tried to split them up a bit by category. No need for that, though. Do you prefer a single core.validator.mapcss file?

No I meant why are there two sections in validator config to add additional files. Anyway if we drop the others, this problem is academic.

This is due to having two separate test classes.

• We need to get the ignore feature back.

That is, setting the description_en test error field?

I think so. We either can make a hash from this and keep the number based approach or directly use the text. Thought the text must be without argument expansion usually, otherwise you can't ignore error groups.

Done in r6649 :-)

• Severity handling for upload is not yet correct (see previous comment)

Which part is incorrect? r6636 basically implements a pre-check to skip tests which generate informational warnings when they are not going to be displayed. A real filtering is still applied later on. This is for performance reasons (to skip the "Crossing Areas" test mostly).

Ah, but still, why don't you use the relevant config upload/non-upload?

Since the individual tests do not know whether they are invoked in standard or upload mode.

• If we allow user written validation checks, then be prepared, that they are made like styles and presets and also stored in zip files. The extension .mapcss is thus a bad choice, as it will conflict with styles
  • presets and styles can be in one zip ATM, even if this is used rarely
  • For validator checks I assume this is more likely to happen

Good point to consider. For instance, we could use *.validator.mapcss (like for *.user.js user scripts in the browser). This does not break available syntax highlighting etc.

Fine.

Mentioned in the documentation ​[1] plus preference dialog (r6651). For the core validation files I'd keep the current file names since they are already in validator/ and to not lose the SVN annotations.

[simon04]

In 6650/josm:

see #9414 see #9542 - MapCSS validator: handle BOM in config files

[simon04]

In 6651/josm:

see #9414 - MapCSS validator preference: display extension *.validator.mapcss

[stoecker]

Mentioned in the documentation ​[1] plus preference dialog (r6651). For the core validation files I'd keep the current file names since they are already in validator/ and to not lose the SVN annotations.

The annotions are no argument - you know "svn mv"?

I think the ignore stuff also needs some other cleanup. I added an note about that in code :-) Probably we should do this after your MapCSS stuff is finished.

I would not mention the old formats in wiki pages. As said - I'd simply drop them and only document the new system - old stuff was never documented outside developer world.

P.S. I like it when older overcomplicated code evolves into something with a cleaner and more powerful structure.

[Don-vip]

Rework of preferences + ignoring of "meta" rules in r6670 (I should have referenced this ticket, too). :)

[stoecker]

Any reason why we keep the old 3 file validator formats. Can they be dropped completely?

[simon04]

Replying to stoecker:

Any reason why we keep the old 3 file validator formats. Can they be dropped completely?

No (not yet). We don't have a correspondence for the "Presets do not contain property key/value" + ignore file nor for the spell checker.

[simon04]

In 6677/josm:

see #9414 fix #9566: MapCSS validator: allow error message to access tags of primitive

For instance, throwWarning: tr("{0} is bad", tag("highway")).

[Don-vip]

In 6687/josm:

see #9414, fix #9576 - "unconnected" pseudo class in MapCSS for nodes without parent way

[Don-vip]

Fixed :) Thanks again Simon for this HUGE improvement, it's awesome :)
If something needs to be done for next tested version, let's open a new ticket :)