Modify

Opened 5 years ago

Closed 4 years ago

#9414 closed enhancement (fixed)

MapCSS-based tag checker/fixer

Reported by: simon04 Owned by: simon04
Priority: major Milestone: 14.01
Component: Core validator Version:
Keywords: mapcss Cc: bastiK

Description

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?

Attachments (0)

Change History (47)

comment:1 Changed 5 years ago by 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" :)

comment:2 Changed 5 years ago by 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 :)

comment:3 in reply to:  2 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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 …

comment:5 Changed 5 years ago by 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?

comment:6 Changed 5 years ago by Don-vip

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

comment:7 in reply to:  6 Changed 5 years ago by simon04

Keywords: mapcss added

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] {...}?

comment:8 Changed 5 years ago by Don-vip

Cc: bastiK added

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

comment:9 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by simon04

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

comment:11 Changed 5 years ago by simon04

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

comment:12 Changed 5 years ago by simon04

In 6511/josm:

see #9414 - MapCSSTagChecker: handle tagging alternatives

comment:13 Changed 5 years ago by simon04

In 6512/josm:

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

comment:14 Changed 5 years ago by simon04

In 6513/josm:

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

comment:15 Changed 5 years ago by Don-vip

In 6529/josm:

Various stuff:

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

comment:16 Changed 5 years ago by 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.

comment:17 Changed 5 years ago by simon04

In 6534/josm:

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

comment:18 Changed 5 years ago by simon04

In 6537/josm:

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

comment:19 Changed 5 years ago by 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

comment:20 Changed 5 years ago by Don-vip

Milestone: 14.01

comment:21 in reply to:  19 Changed 5 years ago by 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 !

comment:22 Changed 5 years ago by simon04

In 6548/josm:

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

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

comment:23 Changed 5 years ago by Don-vip

Well done ! :)

comment:24 Changed 5 years ago by simon04

In 6550/josm:

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

comment:25 Changed 5 years ago by simon04

In 6553/josm:

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

comment:26 Changed 5 years ago by 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 =/!=/~=/^=/$=/*=/=~/!~.

comment:27 Changed 5 years ago by simon04

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

comment:28 Changed 5 years ago by simon04

In 6601/josm:

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

comment:29 Changed 5 years ago by Don-vip

Priority: normalmajor

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

comment:30 Changed 5 years ago by simon04

In 6636/josm:

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

comment:31 Changed 5 years ago by simon04

In 6637/josm:

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

comment:32 Changed 5 years ago by 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.

comment:33 Changed 5 years ago by 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

comment:34 in reply to:  33 ; Changed 5 years ago by 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.

Last edited 5 years ago by stoecker (previous) (diff)

comment:35 in reply to:  34 ; Changed 5 years ago by 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.

Last edited 5 years ago by stoecker (previous) (diff)

comment:36 Changed 5 years ago by simon04

In 6645/josm:

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

comment:37 Changed 5 years ago by 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.*].

comment:38 in reply to:  35 ; Changed 5 years ago by 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.

Last edited 5 years ago by simon04 (previous) (diff)

comment:39 Changed 5 years ago by simon04

In 6650/josm:

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

comment:40 Changed 5 years ago by simon04

In 6651/josm:

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

comment:41 in reply to:  38 Changed 5 years ago by 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.

comment:42 Changed 5 years ago by Don-vip

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

comment:43 Changed 5 years ago by stoecker

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

comment:44 in reply to:  43 Changed 5 years ago by 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.

comment:45 Changed 5 years ago by 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")).

comment:46 Changed 5 years ago by Don-vip

In 6687/josm:

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

comment:47 Changed 4 years ago by Don-vip

Resolution: fixed
Status: newclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.