[PATCH] Don't suggest to change incline=0% to -0% when reversing or combining ways

[Klumbumbus]

What steps will reproduce the problem? ¶

1. have a way highway=secondary incline=0%
2. reverse the way

What is the expected result? ¶

reversed way

What happens instead? ¶

JOSM asks to change incline=0% to incline=-0%

Please provide any additional information below. Attach a screenshot if possible. ¶

from ticket:19485#comment:10

same for incline=0° and incline=0

there are about 25k such combinations

https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/corrector/ReverseWayTagCorrector.java?rev=16438#L145

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-07-07 09:21:17 +0200 (Tue, 07 Jul 2020)
Build-Date:2020-07-08 01:30:50
Revision:16739
Relative:URL: ^/trunk

Identification: JOSM/1.5 (16739 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1909 (18363)
Memory Usage: 784 MB / 1820 MB (334 MB allocated, but free)
Java version: 1.8.0_251-b08, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1680x1050 (scaling 1.0x1.0)
Maximum Screen Size: 1680x1050
Best cursor sizes: 16x16 -> 32x32, 32x32 -> 32x32
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:<java.home>\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=C:\Program Files (x86)\josm-latest.jnlp, -Djnlpx.remove=false, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Djnlpx.splashport=61109, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

[gaben]

It's better to change the regex here: https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/corrector/ReverseWayTagCorrector.java?rev=16438#L209

[gaben]

The patch is based on the modified regex, plus added some test cases. The test cases were swapped I fixed that as well.

[gaben]

Please review and test :)

[stoecker]

• (\\d*[,.]?\\d*)(.*) should be be ([0-9,.]+) or you will loose .1 which is valid.
• Why do you add the sign to the number parser?
• The reversed number should not use a "," anymore if the original had one. Use fixedNum there as well.
• Any reason why you reverse the arguments in the test?
• parseDouble probably throws exceptions which should be catched.

[gaben]

Thanks @stoecker for the quick review!

Replying to stoecker:

• (\\d*[,.]?\\d*)(.*) should be be ([0-9,.]+) or you will loose .1 which is valid.
• Why do you add the sign to the number parser?
• The reversed number should not use a "," anymore if the original had one. Use fixedNum there as well.
• Any reason why you reverse the arguments in the test?
• parseDouble probably throws exceptions which should be caught.

• Try it out :) .1 will become -.1. Just noticed, the current version (wasn't modified since 2009) will leave this as is.
• It doesn't affect the output, but thanks for noticing.
• I didn't want any behaviour change. If you think it should be fixed here, instead of the validator, I can modify the patch.
• Yes. The first parameter is the expected one, the second is the actual, so what the function returns. (Causes confusion in the IDE.)
• Yeah probably when someone gives a really big number, I wanted to ask what to do here :) So what to do? Log somewhere something, or suppress (return the original) or something else?

[stoecker]

• Try it out :) .1 will become -.1. Just noticed, the current version (wasn't modified since 2009) will leave this as is.

Ah sorry. Thought there is a + instead of * for the first numbers.

• It doesn't affect the output, but thanks for noticing.
• I didn't want any behaviour change. If you think it should be fixed here, instead of the validator, I can modify the patch.

Not instead, but also. When we anyway modify values we can also fix them. E.g. boolean values are always converted to yes/no regardless of the original (which I believe is a major reason than true/false and 0/1 vanished from the database).

• Yeah probably when someone gives a really big number, I wanted to ask what to do here :) So what to do? Log somewhere something, or suppress (return the original) or something else?

I'd simply handle it as != 0, i.e. change the sign.

[gaben]

Suggestions applied, improved test cases

[gaben]

Also remove sign from null value, test case updated

[stoecker]

I'd still add a try{} around the parse otherwise it's dangerous.

[gaben]

Exception handling added

[gaben]

I hope this time everything looks good :) If you find anything, please feel free to modify the patch.

[stoecker]

In 16771/josm:

fix #19508 - better reversal function for number, patch by gaben