Modify

Opened 22 months ago

Last modified 21 months ago

#22639 new defect

False validator warning "voltage:* without transformer"

Reported by: Gazer75 Owned by: team
Priority: normal Milestone:
Component: Core validator Version:
Keywords: power transformer voltage Cc: Klumbumbus

Description

Validator complains about voltage:*= as if there was no power=transformer tag on that node, even though its tagged properly.

Attachments (0)

Change History (24)

comment:1 by skyper, 22 months ago

Keywords: power transformer voltage added

comment:2 by skyper, 22 months ago

Actually, the warning asks for an additional tag transformer=* but the message is easily misunderstood.

comment:3 by taylor.smock, 22 months ago

I'm going to assume that the voltage:* keys are as follows:

In each case, the wiki documentation specifies that the key requires a osmwiki:transformer tag, so I don't think the validator is giving a false warning.


Since skyper just added some additional information specifying that the validator message could be confusing, I'll reproduce the truncated mapcss here:

/* {0.key} without {1.key} (warning level) */
node[voltage:primary           ][!transformer],
node[voltage:secondary         ][!transformer],
node[voltage:tertiary          ][!transformer],
// [...snip...]
*[source:addr:housenumber      ][!addr:housenumber] {
  throwWarning: tr("{0} without {1}", "{0.key}", "{1.key}");
  group: tr("missing tag");
  assertMatch: "node source:addr:postcode=postman";
}

I'm not certain how

  • missing tag
    • voltage:primary without transformer
    • voltage:secondary without transformer
    • voltage:tertiary without transformer

is confusing, but maybe I'm missing something.

comment:4 by francois.lacombe, 22 months ago

Agreed, the warning is legit but its content may be improved with kind of "voltage:primary missing transformer=* key" or something.

The message could be confusing as mappers can look for missing power=transformer instead of transformer=*.

I confirm voltage:primary, voltage:secondary and voltage:tertiary require transformer=* and not power=transformer.
power=pole + transformer=* + voltage:primary=* is valid too and only transformer=* is common to both situations.

Mappers confident enough to use voltage:primary can be expected to understand transformer=* too (or dismiss the warning).

Regards

Last edited 22 months ago by francois.lacombe (previous) (diff)

in reply to:  4 comment:5 by taylor.smock, 22 months ago

Replying to francois.lacombe:

The message could be confusing as mappers can look for missing power=transformer instead of transformer=*.

Thanks for the explanation. That makes sense.

For the record, this is the same text we use for:

  • [source:name][!name]
  • [maintenance][!highway]

Should we change the message to be

/* {0.key} without {1.key} (warning level) */
node[voltage:primary           ][!transformer],
node[voltage:secondary         ][!transformer],
node[voltage:tertiary          ][!transformer],
// [...snip...]
*[source:addr:housenumber      ][!addr:housenumber] {
  throwWarning: tr("{0} without a tag for {1}", "{0.key}", "{1.key}");
  group: tr("missing tag");
  assertMatch: "node source:addr:postcode=postman";
}

comment:6 by francois.lacombe, 22 months ago

I fail to translate it accordingly.

Will it give "voltage:primary without a tag for transformer"?

comment:7 by taylor.smock, 22 months ago

Yes. I kind of suck at wording stuff clearly, so I'd appreciate feedback/suggestions on wording.

I'd prefer to not move the voltage:* checks to their own check, but it might be necessary.

comment:8 by francois.lacombe, 22 months ago

I understand the need to remain kind of generic there and I'd prefer to don't move voltage to their own checks.

Shouldn't we use this instead?

throwWarning: tr("{0} without {1}=*");

As to get "voltage:primary without transformer=*"

The main point is to state clearly it misses a key.

comment:9 by taylor.smock, 22 months ago

Looking at source:trunk/resources/data/validator/combinations.mapcss@18622:12-82#L12, I think adding =* will work, it just seems to be extraneous for most of them.

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

comment:10 by skyper, 22 months ago

I know we have a ticket for better highlighting tags/keys/values from placeholders in warnings but do not find it, atm.
Overall, I think this is a general problem and adding =* is just a workaround. Do we need a new placeholder which adds the two symbols automatically?

On a short hand, I would add the two symbols in the placeholder definition but this would make it more difficult once the general problems are fixed.
throwWarning: tr("{0} without a tag for {1}", "{0.key}", "{1.key}=*");

comment:11 by skyper, 22 months ago

Cc: Klumbumbus added

comment:12 by francois.lacombe, 22 months ago

Yes, adding =* was definitely a workaround.

Completely agree to find a more general solution for this kind of problem

comment:13 by taylor.smock, 22 months ago

Honestly, if we know of a preset that would fix the problem, we should just open the preset window for that object. So something like fixPresetWindow('{0.key}', '{1.key}') might do it.

    fixPresetWindow(Env env, String... possibleTags) {
        TagMap tags = ...// Parse tags
        Collection<TaggingPreset> presets = TaggingPresets.getMatchingPresets(env.type, tags, true);
        // Either show a modal with a preset selection list, or just open a single preset window
    }

I'd have to check and see how expensive it would be with something like NSI enabled, but if it takes < 1s to find the preset, it might be worth it.

in reply to:  10 ; comment:14 by Klumbumbus, 22 months ago

Replying to skyper:

I know we have a ticket for better highlighting tags/keys/values from placeholders in warnings but do not find it, atm.

It is #11153, which would be a great improvement in readability of validator messages in general, imho.

Wordings like "{1.key}=*" would make this tag highlighting harder I guess.

in reply to:  14 comment:15 by skyper, 22 months ago

Replying to taylor.smock:

Honestly, if we know of a preset that would fix the problem, we should just open the preset window for that object. So something like fixPresetWindow('{0.key}', '{1.key}') might do it.

    fixPresetWindow(Env env, String... possibleTags) {
        TagMap tags = ...// Parse tags
        Collection<TaggingPreset> presets = TaggingPresets.getMatchingPresets(env.type, tags, true);
        // Either show a modal with a preset selection list, or just open a single preset window
    }

I'd have to check and see how expensive it would be with something like NSI enabled, but if it takes < 1s to find the preset, it might be worth it.

I think the current preset links in Tags/Memberships dialog work similar and most of the time this works atm by selecting the object in validator dialog and then using the preset link of Tags/Membership dialog. Sure, for complex situations this is not sufficient as many preset links might be available which in general makes it more complex especially regarding external presets and rules.

Replying to Klumbumbus:

Replying to skyper:

I know we have a ticket for better highlighting tags/keys/values from placeholders in warnings but do not find it, atm.

It is #11153, which would be a great improvement in readability of validator messages in general, imho.

Thanks, +1

Wordings like "{1.key}=*" would make this tag highlighting harder I guess.

I recognized that placeholders, mean-while, sometimes only use the key or value as "normal" text and not in context of tags. Imho, these messages would have to be adjusted if #11153 is fixed.

comment:16 by taylor.smock, 22 months ago

I think fixing #11153 will also fix the confusion problem brought up in this ticket. Does anyone have a reason to not close this ticket as a duplicate of #11153 in that case?

comment:17 by francois.lacombe, 22 months ago

Ok on me to close this one as duplicate

comment:18 by Gazer75, 22 months ago

If this is actually complaining about missing transformer=* tag then it was quite misleading.

Also the transformer=main should be default value no? So no need to tag.
Saves tag bloat and time when mapping. It is also not a required tag.

in reply to:  18 ; comment:19 by Klumbumbus, 22 months ago

Replying to Gazer75:

It is also not a required tag.

https://wiki.openstreetmap.org/wiki/Key:voltage:primary says that voltage:primary=* requires transformer=*

comment:20 by francois.lacombe, 22 months ago

transformer=* is the only key we can expect to validate usage of voltage:primary, voltage:secondary...

I second Klumbumbus answer: transformer=* isn't a required tag for power=transformer but for voltage:primary, voltage:secondary... and so on.
That was the least complex choice I could made when designing the validation process.

Last edited 22 months ago by francois.lacombe (previous) (diff)

comment:21 by Gazer75, 22 months ago

Guess I'll just permanently ignore this warning in JOSM then. I'm not going to tag 99% of transformers with main value, that's just bloat IMHO.
I do tag aux and generator when appropriate along with the proper primary/secondary value if known.

You can actually have a generator transformer with a higher primary than secondary, though its rare.
Some really big generators can be over 20kV and potentially have a transformer for local distribution connected to it that can be less.

comment:22 by francois.lacombe, 22 months ago

From consumer perspective it's not valuable to default transformer=main as we miss too many any other transformer=* actually tagged.

Implicit data works well until we're unable to make a tiny difference between two features.
Sorry to read it sounds like bloat and again, that was the least complex solution to choose.

in reply to:  19 comment:23 by anonymous, 22 months ago

Replying to Klumbumbus:

Replying to Gazer75:

It is also not a required tag.

https://wiki.openstreetmap.org/wiki/Key:voltage:primary says that voltage:primary=* requires transformer=*

Should probably modify https://wiki.openstreetmap.org/wiki/Tag:power%3Dtransformer#Tagging to be more in line with this as well then.

comment:24 by francois.lacombe, 21 months ago

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to Gazer75.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


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