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

Stop duplicate warnings by Construction2.validator.mapcss #1595

Closed
wants to merge 10 commits into from

Conversation

Famlam
Copy link
Collaborator

@Famlam Famlam commented Oct 9, 2022

Construction2.validator.mapcss and JOSM combinations.mapcss contain practically the same rules and thus always produce a duplicate warning:

Osmose mapcss way[highway][construction][highway!=construction][construction!=minor] is made unnecessary by:
JOSM combinations mapcss L860: way[highway][construction][construction!~/^(yes|minor|no)$/][highway!=construction]

Osmose mapcss way[railway][construction][railway!=construction][railway!=abandoned][railway!=razed][railway!=dismantled][construction!=minor] is made unnecessary by:
JOSM combinations mapcss L861: way[railway][construction][construction!~/^(yes|minor|no)$/][railway!=construction]

I would propose to remove these 2 rules from Osmose so only a single warning remains.

Also, as only tests for proposed highways/railways remain, I would reduce the priority a bit.
(Personal interpretation, but I would say highway=residential + proposed=tertiary means a residential highway that will become tertiary in the future, and should not necessarily be wrong. Furthermore, the JOSM rules have a lower priority than 1, so it would be odd to have rules for proposed at a higher priority than rules for construction. Lastly, proposed is more difficult to verify than actual construction)

Famlam added 2 commits October 9, 2022 20:16
[Construction2.validator.mapcss](https://github.com/osm-fr/osmose-backend/blob/master/plugins/Construction2.validator.mapcss) and [JOSM combinations.mapcss](https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/combinations.mapcss) contain the same rules:

Osmose mapcss `way[highway][construction][highway!=construction][construction!=minor]` is made unnecessary by:
JOSM combinations mapcss  L860: `way[highway][construction][construction!~/^(yes|minor|no)$/][highway!=construction]`

Osmose mapcss `way[railway][construction][railway!=construction][railway!=abandoned][railway!=razed][railway!=dismantled][construction!=minor]` is made unnecessary by:
JOSM combinations mapcss L861: `way[railway][construction][construction!~/^(yes|minor|no)$/][railway!=construction]`

I would propose to remove these 2 rules from Osmose.

Also, as only tests for `proposed` highways/railways remain, I would reduce the priority a bit.
Closes osm-fr#1567

(Also regenerated the python code of indoor.validator.mapcss and Bicycle.validator.mapcss; doesn't effectively change anything as these do not use {n} capture groups, but prevents confusion when we don't remember the recent changes anymore :) )
@Famlam
Copy link
Collaborator Author

Famlam commented Oct 16, 2022

After #1605 is merged, the python needs to be regenerated or it'll generate a merge conflict

@frodrigo
Copy link
Member

I am in favour of depus the rules.

But here we loose the item number. Osmose have a more fine grain categorisation of issues.
There is already -osmose* properties in JOSM MapCSS contrib files. But not in JOSM core MapCSS one.

As the rules in the MapCSS are in a too generic group in the mapcss, we cannot assign an item/class number in the Osmose mapping file.

cc @Klumbumbus

@Famlam
Copy link
Collaborator Author

Famlam commented Oct 17, 2022

I must say that I don't really understand what you mean/request?

jocelynj and others added 8 commits October 19, 2022 15:22
Per the wiki on tag capture: (https://josm.openstreetmap.de/wiki/Help/Validator/MapCSSTagChecker)
"Classes and pseudoclasses do also count."

Rather than counting in mapcss2osmose (which is fairly difficult as some tags get 'cleaned' away (ex `:completely_downloaded`) and the sequence of the tags gets shuffled around for optimization, count it where it's freshly parsed and store the index it as a property of the captured selector.
[Construction2.validator.mapcss](https://github.com/osm-fr/osmose-backend/blob/master/plugins/Construction2.validator.mapcss) and [JOSM combinations.mapcss](https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/combinations.mapcss) contain the same rules:

Osmose mapcss `way[highway][construction][highway!=construction][construction!=minor]` is made unnecessary by:
JOSM combinations mapcss  L860: `way[highway][construction][construction!~/^(yes|minor|no)$/][highway!=construction]`

Osmose mapcss `way[railway][construction][railway!=construction][railway!=abandoned][railway!=razed][railway!=dismantled][construction!=minor]` is made unnecessary by:
JOSM combinations mapcss L861: `way[railway][construction][construction!~/^(yes|minor|no)$/][railway!=construction]`

I would propose to remove these 2 rules from Osmose.

Also, as only tests for `proposed` highways/railways remain, I would reduce the priority a bit.
@Famlam
Copy link
Collaborator Author

Famlam commented Oct 19, 2022

Hmm, something went wrong while trying to sync it with master...
Please let me know what you meant with your previous comment; I'll create a new, clean PR or discard my changes

@frodrigo
Copy link
Member

But here we loose the item number. Osmose have a more fine grain categorisation of issues.
There is already -osmose* properties in JOSM MapCSS contrib files. But not in JOSM core MapCSS one.
As the rules in the MapCSS are in a too generic group in the mapcss, we cannot assign an item/class number in the Osmose mapping file.

Most of all generated plugins from JOSM are in 9xxx items. Thus items are more generic than the Osmose one (all other items). And so the users have less control on selecting content from 9xxx than others.

So switching from pure Osmose to JOSM MapCSS move items/class from detailed items/class to more generic ones.

@Famlam
Copy link
Collaborator Author

Famlam commented Oct 19, 2022

So WontFix?

@frodrigo
Copy link
Member

So WontFix?

I am in favour of depus the rules.

I will be better to depus. But it is not so easy. I think we need a talk with JOSM on this.

@Klumbumbus
Copy link

I will be better to depus.

What means "depus"?

I think we need a talk with JOSM on this.

I guess there are much much more overlaps of the JOSM rules and the Osmose rules anyway, so these two of this issue don't matter that much and could also be kept in Osmose.

Maybe in Osmose a more fine categorization of the JOSM rules can be done by the "group" property of the JOSM core rules?

@frodrigo
Copy link
Member

I will be better to depus.
What means "depus"?

Sorry: depublicate.

I think we need a talk with JOSM on this.
I guess there are much much more overlaps of the JOSM rules and the Osmose rules anyway, so these two of this issue don't matter that much and could also be kept in Osmose.
Maybe in Osmose a more fine categorization of the JOSM rules can be done by the "group" property of the JOSM core rules?

But here, eg, construction have it own item and tags in Osmose MapCSS https://github.com/osm-fr/osmose-backend/blob/master/plugins/Construction2.validator.mapcss#L39 https://github.com/osm-fr/osmose-backend/blob/master/plugins/Construction2.validator.mapcss#L25

While in JOSM https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/combinations.mapcss#L865 it is in a relatively generic group "suspicious tag combination"

We already have group support. We even can map group to Osmose item/class https://github.com/osm-fr/osmose-backend/blob/master/mapcss/item_map.py#L145 .

But as you can see here. JOSM group are too generics for Osmose.

@Klumbumbus
Copy link

We already have group support.

Ok, I didn't know that, because it seems you cannot filter by these groups with the checkboxes on the left of the Osmose map.

But as you can see here. JOSM group are too generics for Osmose.

Yes, I understand the problematic, but I don't think we will split up the rules in JOSM core just to add the "-osmoseItemClassLevel" properties.

@frodrigo
Copy link
Member

We already have group support.

Ok, I didn't know that, because it seems you cannot filter by these groups with the checkboxes on the left of the Osmose map.

Checkbox are at item level = one josm MapCSS file
So we use group at class level (can be see like sub-item).
We can also filter on class, but not available directly on the UI.

But as you can see here. JOSM group are too generics for Osmose.

Yes, I understand the problematic, but I don't think we will split up the rules in JOSM core just to add the "-osmoseItemClassLevel" properties.

The point is not about splinting. But eg here "suspicious tag combination" is declared in may rules. So in Osmose (like JOSM) the results are grouped into the same class. That the point. We cannot map a rule, part of a group to a specific Osmose Item/class.

I don't know, if on JOSM side you are open to add "-osmose*" to core JOSM MapCSS Validation files.
Maybe there is better solution. Talk, few years ago with @don-vip, he tell me it may have a plan to split away validation rules from JOSM and have a more shared project of validation rules with other tools (for sure like Osmose).

@Famlam
Copy link
Collaborator Author

Famlam commented Nov 5, 2022

Filed an issue on the frontend issue tracker as that'll have to be updated first. I'll remove my PR as it's clear that it'll not be merged in the current form

@Famlam Famlam closed this Nov 5, 2022
@Famlam Famlam deleted the patch-construction2 branch November 5, 2022 19:58
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.

4 participants