mapcss: add unit conversion for length values

[skyper]

A first step was added in r19089, see 23621#comment:13:

Copied from 23621#comment:15:

First impressions seems to be good, but I'm a bit puzzled on some things and have some questions on other things:

1) why does it return - on invalid inputs? This makes it fairly prone to errors like:

node[height ^= "-"] {
  throwError: tr("Surprisingly low value of {0} m", to_float(siunit_length(tag(height))));
}

which crashes JOSM if validating an object with height=-1 (or gives a bad message if to_float is omitted, just included it here to showcase a worst case scenario). Why not returning null or so? The example mapcss is hypothetically by the way, not a suggestion :)

2) why not supporting negative numbers? For validation purposes, this makes a lot of sense, for instance the key ele has a lot of negative values.

3) as it's a conversion, wouldn't to_SIunit_length or so be a better name than siunit_length?

4) probably it should also support km and nmi as they're explicitly listed on the ​wiki. Even though they're the defaults and could hypothetically be omitted, some people do include them :)

5) as it's always a Double, why not return it as a Double instead of a String 0:) ?

Bumping the priority a bit since it's almost release day with this new function :)

[skyper]

It would be nice if the notation of feet plus inches like 6'7'' would also be supported without splitting the values manually prior to converting.

[stoecker]

6'7" is supported, 6'7'' not.

[stoecker]

In 19092/josm:

fix #23702, see #23621 - improve length unit conversion

[stoecker]

NOTE: This can produce ugly results due to the conversions. Normally that function should have a rounding possibility which rounds the final results to an approximate similar accuracy.

I.e. when input value has 2 significant digits, the result should never have more than 3.

Because it is a lot of work to get that right (especially for the ft+in combo), I leave that to a future update in case it is really needed.

[stoecker]

Hmm, I don't know what PMD wants:

​https://github.com/JOSM/josm/commit/86e4d775139717e88166dc0245ad517a409cb322#annotation_22274148759

Design/AvoidUncheckedExceptionsInSignatures: src/src/org/openstreetmap/josm/tools/Utils.java#L2072
https://pmd.github.io/pmd-6.55.0/pmd_rules_java_design.html#avoiduncheckedexceptionsinsignatures

Working link: ​https://pmd.github.io/pmd/pmd_rules_java_design.html#avoiduncheckedexceptionsinsignatures

Anybody else with a fix?

[stoecker]

@Taylor: Any idea?

[taylor.smock]

Literally what it says: don't put unchecked exceptions in signatures.

I would tend to disagree with the sentiment -- no, the consumer doesn't have to handle the exception, but it is nice to know where the exception might be occurring. I'll have to check and see if IDEs look at @throws or just the method throws definition.

I'll also check and see if PMD 7.3.0 is less chatty with our code base -- it looks like that check was disabled in 7.0.0 (although it might be re-enabled by now).

src/org/openstreetmap/josm/tools/Utils.java

@@ -2069 +2069 @@
      * @throws IllegalArgumentException if input is no valid length
      * @since 19089
      */
-    public static Double unitToMeter(String s) throws IllegalArgumentException {
+    public static Double unitToMeter(String s) {
         s = s.replaceAll(" ", "").replaceAll(",", ".");
         Matcher m = PATTERN_LENGTH.matcher(s);
         if (m.matches()) {

[stoecker]

Hmm, so they suggest "@throws" instead of "throws" and not in addition to it because Java does not enforce catching them. One possible view, but I'm not really convinced. I'd rather mark it for ignore ;-)

[taylor.smock]

It is still a problem with pmd 7.3.0, but it at least fixed the FPs for static imports (that was an issue with 7.0).

I'll go ahead and do a dependency update Thursday -- there are a lot of other "new" issues in 7.x that I need to go through and fix or ignore, so it isn't going to be a small dependency update.

The big one to ignore/properly configure is going to be MethodNamingConvention -- it pretty much flags everything in mapcss Functions.

If you want to see the "new" issues from PMD 7.2.0 as compared to PMD 6.55.0, here is a patch:

tools/ivy.xml

@@ -20 +20 @@
         <!-- proguard->default -->
         <dependency org="com.guardsquare" name="proguard-ant" rev="7.4.2" conf="proguard->default"/>
         <!-- pmd->default -->
-        <!-- PMD 7.0.0 has too many false positives right now. When updating, don't forget to add pmd-ant as a new dependency -->
-        <dependency org="net.sourceforge.pmd" name="pmd-core" rev="6.55.0" conf="pmd->default"/>
-        <dependency org="net.sourceforge.pmd" name="pmd-java" rev="6.55.0" conf="pmd->default"/>
-        <dependency org="net.sourceforge.saxon" name="saxon" rev="9.1.0.8" conf="pmd->default">
-            <artifact name="saxon" type="jar"/>
-            <artifact name="saxon" type="jar" maven:classifier="dom"/>
-        </dependency>
+        <dependency org="net.sourceforge.pmd" name="pmd-core" rev="7.2.0" conf="pmd->default"/>
+        <dependency org="net.sourceforge.pmd" name="pmd-ant" rev="7.2.0" conf="pmd->default"/>
+        <dependency org="net.sourceforge.pmd" name="pmd-java" rev="7.2.0" conf="pmd->default"/>
+        <dependency org="org.xmlresolver" name="xmlresolver" rev="6.0.4" conf="pmd->default"/>
         <!-- spotbugs->default -->
         <dependency org="com.github.spotbugs" name="spotbugs" rev="4.8.4" conf="spotbugs->default"/>
         <dependency org="com.github.spotbugs" name="spotbugs-ant" rev="4.8.4" conf="spotbugs->default"/>

[stoecker]

I have no issue with PMD messages going up. No need to hold back a version upgrade only to cleanup. We can do so after the upgrade.

[taylor.smock]

Ordinarily, I'd agree with you. Except the PMD version bump adds ~830 new issues. I'd rather go through them and fix/ignore before application, especially since I'm not going to do that until Thursday.

Here is a breakdown of the "new" issues:

Count	Name	Notes
1	AvoidUncheckedExceptionsInSignatures
7	ConfusingArgumentToVarargsMethod
15	ConsecutiveAppendsShouldReuse
331	FieldNamingConventions	A good chunk is from xml deserialization; I'll probably whitelist those classes
1	FinalFieldCouldBeStatic
1	InefficientStringBuffering
8	LambdaCanBeMethodReference
1	LongVariable
67	MethodNamingConventions	Mostly from MapCSS Functions.java
2	PreserveStackTrace
2	ShortMethodName	This is actually a local-only problem due to out-of-tree patches I have
4	SignatureDeclareThrowsException
66	SimplifyBooleanReturns
21	UnnecessaryBoxing
258	UnnecessaryFullyQualifiedName
8	UnnecessaryModifier
10	UnnecessaryReturn
1	UnnecessaryVarargsArrayCreation
1	UnusedNullCheckInEquals
1	UnusedPrivateField
1	UseArraysAsList
22	UseDiamondOperator
3	UseExplicitTypes
1	UseIndexOfChar
1	UselessQualifiedThis

Some of them will be easy to fix, others will be a bit more involved, and some will require configuration changes (FieldNamingConventions specifically).

EDIT: It looks like we are ignoring some of them already (specifically FieldNamingConventions). I suspect they got moved around.