add MaxweightExtraValue

[Marc-marc-marc]

ignore (don't raise an issue) some text values describing the absence of a specific maximum weight limit

[Famlam]

I had three concerns about this PR:

1. It's a PR on master, not on the dev branch ;)
2. Mind that e.g. maxweight:hgv, a common key (unlike maxheight:*), isn't handled by this implementation. Better is probably to either split by :, or to use startsWith, or just check if maxweight is "in" the key
3. "default", "below_default", "none", "no" don't seem to be defined values for maxweight (well, none is for :conditional keys, but that's not checked here). So I would remove these.

[Marc-marc-marc]

1. It's a PR on `master`, not on the `dev` branch ;)

done

2. Mind that e.g. `maxweight:hgv`, a common key (unlike `maxheight:*`), isn't handled by this implementation. Better is probably to either split by `:`, or to use `startsWith`, or just check if `maxweight` is "in" the key

it would indeed be better to include, I'll let you see which key selection method is best, because I suppose it's better to have the same logic for the 2 tests.
suggestion: validate without hgv to do a quick iteration, that won't prevent you from making another improvement for hgv

3. `"default", "below_default", "none", "no"` don't seem to be defined values for [maxweight](https://wiki.openstreetmap.org/wiki/Key%3Amaxweight) (well, `none` is for `:conditional` keys, but that's not checked here). So I would remove these.

all values have at least one occurrence of maxweight and also with suffix for ex 1 617 maxweight:bus=none

[Famlam]

done

Thanks

---

suggestion: validate without hgv to do a quick iteration, that won't prevent you from making another improvement for hgv

Sure, I can do a follow-up PR, but I can also just give you the solution now ;)
-> Replace tag == "maxweight" by tag.split(":", 1)[0] == "maxweight"
The other two I had in mind would also trigger on maxweightrating, which would be wrong.

---

I suppose it's better to have the same logic for the 2 tests

Only if it makes sense of course. For maxweight I can understand that e.g. maxweight:hgv and maxweight:bus differ, since that could be about weight distribution (a bus being much lighter than a truck filled with concrete). Or just a way to ban heavy vehicles to avoid vibrations from passing hgv's in residential areas. So maxweight:hgv=14 + maxweight:bus=unsigned would be logical to me. (Although I personally never tag unsigned, but I understand people doing so)

For maxheight on the other hand, maxheight:hgv = default/none/unsigned wouldn't be logical, because why would a specific vehicle type be able to pass under different heights than others? So this is very likely just redundant tagging of the default (e.g. maxheight:hgv=default + maxheight=default). Probably also why these tags almost don't exist in the database. So I don't see the reason to accept it on this key. If you have an example, please proof me wrong and I'll happily change my mind ;)

---

all values have at least one occurrence of maxweight and also with suffix for ex 1 617 maxweight:bus=none

I don't think we should support the spread of random values, especially ones with duplicate meanings. Allowing none I can imagine, since it's documented for :conditional. However maxweight=no only exists 2x, maxweight=default and maxweight=below_default each exists only once. So that's probably just people mistakenly not knowing that these should be none instead, or an actual value. So warning for these undocumented tags (and a few more similar ones with the same meaning) helps avoiding the accidental use of undesired duplicate-meaning values.
Just looking at maxweight:bus, I see that there's already a lot of duplicate-meaning values: none being the most common one (1617x), but few occasions of no (98x), unrestricted (75x), no_limit (13x). Probably just people making up their own tags. I'm very happy to keep warning for those latter three as they should become unsigned (or none) instead.

[Marc-marc-marc]

suggestion: validate without hgv to do a quick iteration, that won't prevent you from making another improvement for hgv

Sure, I can do a follow-up PR, but I can also just give you the solution now ;) -> Replace tag == "maxweight" by tag.split(":", 1)[0] == "maxweight" The other two I had in mind would also trigger on maxweightrating, which would be wrong.

yes but your rules doesn't just add a match for the valid maxweight:hgv key, but also test several wrong keys (maxweight:source maxweight:total maxweight:note), strange (maxweight:relative maxweight:except maxweight:physical) or that don't need to have this check (maxweight:signed)

I suppose it's better to have the same logic for the 2 tests

Only if it makes sense of course. For maxweight I can understand that e.g. maxweight:hgv and maxweight:bus differ,

by logic, i meant either you list a series of documented/common keys (and you have to update the list from time to time), or you accept all the keys with a prefix (and you're going to give incorrect advice for incorrect keys such as *:note *:source or the others listed before). you have the same problem for the 2 keys, which is why i thought it would be good to have the same code between the 2, even if the list of valid suffixes varies between the 2.

For maxheight on the other hand, maxheight:hgv = default/none/unsigned wouldn't be logical, because why would a specific vehicle type be able to pass under different heights than others?

I never see this sign for maxheight (I don't code maxheight :p)
but for mayweight, I already see ‘x tonnes except psv’ or ‘except destination’

all values have at least one occurrence of maxweight and also with suffix for ex 1 617 maxweight:bus=none

I don't think we should support the spread of random values, especially ones with duplicate meanings. Allowing none I can imagine, since it's documented for :conditional.

for me it's a common misttake (but I don't want to raise an issue for that) : "none @" is a rule for :conditional, for key outside for conditional, the negative value is no (fee=no lit=no, whellchair=no, ...) and not "none"

the problem is that it's not documented, so I don't want osmose to consider that it's ‘none’ and another tool may consider that it's ‘no’, and another in favor of "default". that's why i've listed the 3, to concentrate on what's consensual.
But I won't be offended if you edit the PR to make the test stricter.
However, I think it would be useful to validate the reduction in non-conflicting issues while considering the unique or non value for ‘none/no/default/unsigned’ (and I think this discussion would be better placed on the tagging list).

[Famlam]

yes but your rules doesn't just add a match for the valid maxweight:hgv key, but also test several wrong keys (maxweight:source maxweight:total maxweight:note), strange (maxweight:relative maxweight:except maxweight:physical) or that don't need to have this check (maxweight:signed)

That's not correct: the only keys considered are the ones that consist of [key][optional :vehicle_type][optional :direction]. See lines 81-82 of the code for the combinations.

by logic, i meant either you list a series of documented/common keys (and you have to update the list from time to time), or you accept all the keys with a prefix (and you're going to give incorrect advice for incorrect keys such as *:note *:source or the others listed before). you have the same problem for the 2 keys, which is why i thought it would be good to have the same code between the 2, even if the list of valid suffixes varies between the 2.

I can't fully follow what you mean, but I assume this is answered by my previous reply: *:source etc aren't tested?
Of course the exceptions shouldn't be the same for both: following that logic we'd also have to allow maxspeed=below_default (as maxspeed is another rule with exceptions). As a driver, I'd be very scared to go over a bridge with a below_default maxweight. Will it break or not, I can't see that beforehand. Unlike maxheight, where I can visually judge this (provided my speed is low enough)

I never see this sign for maxheight (I don't code maxheight :p)
but for mayweight, I already see ‘x tonnes except psv’ or ‘except destination’

Ok, then I misunderstood your comment, please ignore what I said, probably we mean the same. I was under the impression you also wanted to change the other code line.

for me it's a common misttake (but I don't want to raise an issue for that) : "none @" is a rule for :conditional, for key outside for conditional, the negative value is no (fee=no lit=no, whellchair=no, ...) and not "none"
the problem is that it's not documented, so I don't want osmose to consider that it's ‘none’ and another tool may consider that it's ‘no’, and another in favor of "default". that's why i've listed the 3, to concentrate on what's consensual.
But I won't be offended if you edit the PR to make the test stricter.
However, I think it would be useful to validate the reduction in non-conflicting issues while considering the unique or non value for ‘none/no/default/unsigned’ (and I think this discussion would be better placed on the tagging list).

Yes, none is in principle only for *:conditional (per the wiki), and per the wiki unsigned should be used when not signed. I don't see no, default or below_default on the wiki and thus wouldn't want to start allowing them. If you're aware of such a tool that promotes maxweight=default please let me know so I can reach out to them, but usage (1x on all of OSM) makes me think this tool probably doesn't exist?
IMO we (Osmose) shouldn't start making up "allowed" values "as a prevention" only because they exist once in the database. As mentioned, otherwise we'd have to whitelist all of no | none | unsigned | default | no_limit | unrestricted and probably a few more, just because there's somewhere someone who added it. And if there's no warning for it anymore, people will not notice they use an uncommon value, so it will not be fixed. After a discussion on the wiki/tagging list/... of course we should whitelist what is agreed upon.

[Marc-marc-marc]

yes but your rules doesn't just add a match for the valid maxweight:hgv key, but also test several wrong keys (maxweight:source maxweight:total maxweight:note), strange (maxweight:relative maxweight:except maxweight:physical) or that don't need to have this check (maxweight:signed)

That's not correct: the only keys considered are the ones that consist of [key][optional :vehicle_type][optional :direction]. See lines 81-82 of the code for the combinations.

ok.

If you're aware of such a tool that promotes maxweight=default

SC it promotes or promoted default below_default for maxheight for exemple

After a discussion on the wiki/tagging list/... of course we should whitelist what is agreed upon.

I understand your logic, but I think we also need to limit the number of ‘not very useful’ issue : in my opinion, no <> none is not very useful in the sense that a datause has to take both cases into account anyway, given the number of occurrences.
If the community manages to agree, I think it's more useful to do a mass edition to resolve all the cases in 1x and have the cases requiring human review in osmose (or else have a new theme ‘not mechanical fixable’ <> ‘mechanical fixable’ ).

[Famlam]

SC it promotes or promoted default below_default for maxheight for exemple

I don't dispute maxheight=below_default (it's logical AND documented), unlike maxweight=below_default what we're talking about. For maxweight it's not documented AND doesn't make sense AND occurs only once in the full OSM database.

I understand your logic, but I think we also need to limit the number of ‘not very useful’ issue

I don't think this has any notable impact. It involves (if I count quickly maxweight=* and the most popular maxweight:*=*) approx 1893 items worldwide (of which the vast majority, 1605x, is maxweight:bus=none) but it points at very valid issues, that would easily go unnoticed unless warned about.
Especially the (below_)default ones you whitelisted, which have a worldwide impact of 2(!!!) detections, one of each. What are you trying to achieve by whitelisting them if "reducing not very useful issues" is your goal? Actively whitelisting 2 detections of a bad tag to reduce 'not very useful' issues? Why?

in my opinion, no <> none is not very useful

Note I'm not proposing to swap them. I'm proposing to keep flagging them so that a mapper goes out and has a look what's meant. Most likely 'unsigned'. Or maxweight is a typo for maxheight in those cases. So very valid for a field-check because it's a strange value.

in the sense that a datause has to take both cases into account anyway [...]

Only if they're common keys with meaningful values. Not if they're likely (undocumented) typos. And it's of course fully up to the data users to decide what they support, they can even support the French words for unsigned and no and no limit and so on if they wish, but that's offtopic here.

Whether something should be fixed by a mass edit I'll leave out of the discussion, but if you wish to reduce detections, there's plenty of much better candidates for that with much larger impacts, like the old parking:lane:* scheme, sidewalk=none->no, etc .... And basically all the brand:* or wikidata additions from the NameSuggestionIndex. Those have major impact ànd in the latter case would even make sense to repeat frequently. But that's all off-topic.

[Marc-marc-marc]

unlike maxweight=below_default what we're talking about. For maxweight it's not documented AND doesn't make sense

I think it's completely irrelevant to decide here how the community should tag a bridge that we consider incapable of supporting the normal weight.
I had left the value of the previous rule because my aim was not to make a PR to discuss this arbitrarily here.
but I've removed it if it makes it so difficult to validate the subject of this RP.

What are you trying to achieve by whitelisting them if "reducing not very useful issues" is your goal?

I didn't write the list of exceptions in the previous rule, I simply kept most of the values for consistency.

in my opinion, no <> none is not very useful
Note I'm not proposing to swap them. I'm proposing to keep flagging them so that a mapper goes out and has a look what's meant. Most likely 'unsigned'. Or maxweight is a typo for maxheight in those cases. So very valid for a field-check because it's a strange value.

I've added this type of value myself and I don't see what else I can do (maxweight on a bridge or on a highway is not a typo) other than flag them as false positives, which I find counterproductive in terms of human time and the quality of the issues compared to having a rule in the code.
But either way, it's gone.

[Marc-marc-marc]

requested change commited, not because it's good one (deletion after deletion, the PR ends up whitelisting only 5 occurrences for the key maxweight:bus, whereas 95% of the uses of this key are related to the traffic sign ‘x tons exept bus, which makes the remain part of this PR futile)), but to avoid the residual part being blocked.
note that wiki also document that There is also maxweight=none and also documented no as the general negative value.
I send an email to tagging mailing to request more feedback about how to tag it

[Famlam]

note that wiki also document that There is also maxweight=none

Please cite the full line next time, the next few words on the wiki are pretty important:

There is also maxweight=none, but it is misleading as there are also default max weight of vehicles set by law, it is unlikely to find place where one is actually allowed to drive vehicles without any weight limits. However, it is the value used with maxweight:conditional=none @ * so should be treated as meaning defaults apply.

---

and also documented no as the general negative value.

Didn't find a line about the value no on the wiki of maxweight

I send an email to tagging mailing to request more feedback about how to tag it

Great, once the wiki is updated after that discussion and stable, more than happy to see new entries being added!