Modify

Opened 7 years ago

Last modified 2 months ago

#10226 new enhancement

Verify multiple equal values

Reported by: naoliv Owned by: team
Priority: normal Milestone:
Component: Core validator Version:
Keywords: mapcss multiple values repetition Cc: bastiK, simon04

Description

Could JOSM verify for multiple equal values?
For example, ref allows multiple values but if we have ref=A1;A1 we do not see any warning.

Since ref allows multiple values, it should warn only when we have two or more equal values in the set.

Could also be expanded for other keys that allow multiple values.

Attachments (0)

Change History (9)

comment:1 Changed 7 years ago by Don-vip

Cc: bastiK simon04 added

it could be interesting for:

  • source, source:*, attribution, note, description
  • name, *_name
  • ref, *_ref
  • destination, addr:housenumber, voltage, traffic_sign, cuisine

Could we do that using MapCSS? We already have source:trunk/resources/data/validator/multiple.mapcss

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

comment:2 Changed 7 years ago by mdk

In the first moment I was a little bit uncertain, if we could define a general rule, like

  • split() values at ';'
  • trim() the values (values are separated by ";" or by"; " like tracktype=grade1;grade2 and tracktype=grade1; grade2). Perhaps an other possible validator check, because the space after ";" is not correct (as far as I understand the wiki) and should be detected by the normal "value starts with space" rule.
  • find duplicates

There are several cases where we have more than one value for a key:

  1. a list of values (which are not mutually exclusive) like cuisine=kebab;pizza. In this case it makes never sense for multiple equal values.
  2. mutually exclusive values make never sense (like lit=yes;no). This could be an other validation test, if we could identify mutual exclusive values :-).
  3. an array of values like *:lanes=*. In this case equal values make sense, but we have a different delimiter '|'. Also in this case there could be values separated by ';' like turn:lanes:forward=left;through|through|through;right.

For the 3rd case I can construct a false positive:
In turn:lanes:forward=left;through|left;through|left;through|right "through|left" appears twice!
We could solve this problem by independently checking each '|' separated value:

for (String arrayValue : value.split("|")) {
  HashSet<String> values = new HashSet<>();
  for (String value : arrayValue.split(";") {
    value = value.trim();
    if (values.contains(value) {
      warn("multiple equal value " + value);
    } else {
      values.add(value);
    }
  {
}

Are there more than these 3 scenarios?

Is there any other scenario where we could have a false positive?

Last edited 2 years ago by Don-vip (previous) (diff)

comment:3 Changed 7 years ago by Aun Johnsen <lists@…>

oneway=-1/0/1/yes/no/true/false, damn many options there

comment:4 Changed 7 years ago by mdk

But oneway=* is a bad example, because multiple values makes no sense on this key. Nevertheless you will find several values like "yes;no", "no;yes", -1;yes" or "yes;-1". I try to correct them the the last months, but they pop up all the time.

I analysed some cases:

Way 315704369: Exists in the first version, but I don't think the mapper has really added oneway=yes;-1 (created_by=iD 1.6.2)
Is it possible, that iD deletes the old ways (and their history) when you merge two ways?
Is it possible, that iD merges values without warning, when you merge ways?

Way 256185821: Exists in the 3rd version. It looks like the mapper merge a fragmented roundabout into a single closed way (number of nodes changes from 4 to 21 and junction=roundabout is added. But also oneway=yes changes to oneway=yes;-1 (iD 1.6.2)
I thing again, that iD automatically merges the values.
BTW: does it make sense to have junction=roundabout and oneway=* on the same way? Or should JOSM warn about such a situation?
Similar: way 67550887 (iD 1.6.2)

Way 310599559: 1st version directly with oneway=yes;no (JOSM/1.5 (7588 es))
It's the result from a split of way 78560464, which also had oneway=yes;no, but was changed into oneway=yes.
The original way 78560464 is again the result of a merge where oneway=yes changes to oneway=yes;no (iD 1.5.4)
The last change, which produces the way 310599559 is a manual revert of the merge, where oneway=yes;no is left on the new created way.
Again id has merged the values on merging ways.
But JOSM did not warn warn about oneway=yes;no. The validator only creats an info, which is normally not shown on upload:
Value 'yes;no' for key 'oneway' not in presets. - Presets do not contain property value (1)

Conclusion:

  • iD should stop merging values (without warning the user)!
  • JOSM should warn about multiple values on oneway (or more general on keys which don't allow multiple values)
  • JOSM may warn about oneway=* together with junction=roundabout

comment:5 in reply to:  4 ; Changed 7 years ago by naoliv

Replying to mdk:

  • iD should stop merging values (without warning the user)!

The reason I *really* dislike iD is because of things like this:

We're not going to add an intrusive warning to iD for this case. Warnings may be well intentioned, but they run counter to iD's goal of encouraging new users to contribute to the map because they make them feel insecure, even when their edits are perfectly legitimate. "I might break something, I better not touch this map" -- and they walk away. Not to mention they are an annoyance for experienced mappers, who think "of course I want to do what I said I wanted to do", and click through without reading. In the majority of cases, merging with ';' is the correct behavior; our plan for covering the remaining cases is outlined in #908 (adding an unobtrusive warning).

https://github.com/openstreetmap/iD/issues/1480

There are other examples about this too.

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

Replying to naoliv:

Replying to mdk:

  • iD should stop merging values (without warning the user)!

The reason I *really* dislike iD is because of things like this:

We're not going to add an intrusive warning to iD for this case. Warnings may be well intentioned, but they run counter to iD's goal of encouraging new users to contribute to the map because they make them feel insecure, even when their edits are perfectly legitimate. "I might break something, I better not touch this map" -- and they walk away. Not to mention they are an annoyance for experienced mappers, who think "of course I want to do what I said I wanted to do", and click through without reading. In the majority of cases, merging with ';' is the correct behavior; our plan for covering the remaining cases is outlined in #908 (adding an unobtrusive warning).

https://github.com/openstreetmap/iD/issues/1480

There are other examples about this too.

Hope, they do not destroy relations this way.

Never understood, why you need "merge ways" in iD. It is simply no task for new contributes.

comment:7 in reply to:  6 ; Changed 7 years ago by anonymous

Replying to skyper:

Hope, they do not destroy relations this way.

https://github.com/openstreetmap/iD/issues/1461 et al

comment:8 in reply to:  7 Changed 7 years ago by skyper

Replying to anonymous:

Replying to skyper:

Hope, they do not destroy relations this way.

https://github.com/openstreetmap/iD/issues/1461 et al

Well, this is another bug but my concern was about one way being part of an relation and another one not being part. If you merge these ways you damage the relation.

comment:9 Changed 2 months ago by skyper

Keywords: multiple values repetition added

With #15751 in mind, it might be useful to have a separate test for oneway value. I still like the idea of this ticket, though, and maybe we can fix it within the next seven years.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to naoliv
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


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

 
Note: See TracTickets for help on using tickets.