-
Notifications
You must be signed in to change notification settings - Fork 164
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
Update power presets following transformer tagging extension #447
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. Thanks for this update! In general it looks good, but please find some comments inline and below:
Some voltage keys should also be discouraged. Is it possible to raise a warning regarding the following please ?
voltage=* in combination with power=transformer
The question is what should be done with the value of the tag if it is found? It surely should not be removed, because it can still be useful to determine the (proper) values for voltage:primary/secondary… In my opinion this is not a good fit for a validation in iD itself, but rather something for an external QA tool like osmose or maproulette.
[voltage-high=*](https://wiki.openstreetmap.org/wiki/Key:voltage-high) [voltage-low=*](https://wiki.openstreetmap.org/wiki/Key:voltage-low)
These are included in the deprecation rules you provided.
Furthermore, voltage:primary, voltage:secondary, voltage:tertiary are valid with (already in presets) and should require transformer=* (not in validator) to be used.
https://wiki.openstreetmap.org/wiki/Tag:power%3Dtransformer#Voltage_tagging
Similar question as above: If iD can't provide is no good "upgrade" approach, and removing the data would add more harm than leaving it as it, it would be better to implement such a check in an external QA tool. Also, iD shows users the transformer
field to fill in: What more do you expect?
transformer=* should only be used on nodes (it doesn't raise a warning on area currently), just like power=transformer (which accurately raises a warning when used on areas).
You mean when a transformer
tag is present, but no power=transformer
? I would estimate that that is quite rarely the case (because it would have had to be manually added directly in the tag editor by an "experienced" mapper)…
Hi @tyrasd and thank you for the comments. Here are additional answers complementary with previous discussion upside.
Given problem is we don't know what voltage=* means in combination with transformers. It can be primary, secondary, tertiary or even a list of them. That's why it should not be used.
I'd say iD can put the greatest voltage:primary, voltage:secondary or voltage:tertiary in voltage=* and drop others, if possible.
It's not a matter of fields, but a matter of combinations.
That's right, we can keep that apart for this PR. |
I'll resolve change requests in a few hours, wait for it. |
9d3204d
to
49f529e
Compare
Any news on this PR @tyrasd? |
I see. Can you please create a new feature request for this on the iD repo? |
Thank you @tyrasd for review and final validation. Be sure I will report the need to have dependency in tagging rules in iD repo. Best regards |
"main": "Forwards power", | ||
"distribution": "Feeds final consumers, installed outside substations", | ||
"generator": "Steps-up voltage in power plants", | ||
"converter": "Feeds power converters", | ||
"phase_angle_regulator": "Regulates phase angle", | ||
"auxiliary": "Feeds internal systems in substations", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were the previous strings incorrect? The new strings can end up being so lengthy in translation that they ideally would be displayed as tooltips, though I’m unsure if schema-builder supports localizable combo option tooltips yet. For now, perhaps each option could start with the short name, like “Distribution: Feeds final consumers, installed outside substations”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were probably not incorrect, but I thought that having an almost 1:1 copy of the raw tag value isn't a big help for mappers (otherwise this field could have been a regular combo field), that's why I asked for more descriptive labels.
👍 for adding the tag values as part of the labels, I've tweaked it in a33d777 – does that look better?
I propose theses changes following power transformer tagging extension.
It's my first PR here, I hope I didn't break anything and accurately followed contributing rules.
I didn't manage to introduce theses requirements in presets. Can someone help me please?
https://wiki.openstreetmap.org/wiki/Tag:power%3Dtransformer#Voltage_tagging
Closes #415