Add function convert_to_int_float_str

[taldcroft]

Description

This adds a function convert_to_int_float_str() to replace various versions of _coerce_type that exist in Ska3 code. I discovered that the canonical version in parse_cm.common has a serious bug:

In [1]: from parse_cm.common import _coerce_type

In [2]: _coerce_type("01234")
Out[2]: '01234'

In [3]: type(_coerce_type("01234"))
Out[3]: str

The issue is that 01234 is not a valid int literal because of the leading 0. The new function here handles all the test cases correctly.

The intent is to replace all calls to various flavors of _coerce_type with a call to convert_to_int_float_str.
https://github.com/search?q=org%3Asot+_coerce_type&type=code
Though in fact the different flavors seem to mostly be in deprecrated / unused code apart from parse_cm itself.

Interface impacts

None.

Testing

Unit tests

• Mac (new unit tests)

Independent check of unit tests by Javier

• Linux

Functional tests

[jeanconn]

I'd just note in passing that I disagree a bit that the original implementation is a bug. Honestly, maybe obsids should just be strings, but I can see the need for different supporting code if the calling code expects that '01234' -> 1234.

[taldcroft]

@jeanconn - our code base assumes in many places that obsid is a numeric value. Think of all the if obsid < 38000: type checks. And from the bible OP19: OBS, ID=integer, TARGET=(ra, dec, name)|MANEUVER=(v1, v2, v3, angle, ref)....

So obsid is an integer (or maybe a float in the non-compliant real world), not a string.

[taldcroft]

And of course the serious bug is an inconsistent return type for obsid, depending on whether it happens to have a leading 0. No user would expect that and I'm sure it would break code.

[taldcroft]

Also note that the original _coerce_type is being used in many places to parse text, with most of it having nothing to do with obsid.

[jeanconn]

While our code does assume many places that obsid is an integer, if we like to represent it as '01234' that is not an integer. And for OP19, I assume '01234' would be non-compliant. But I'm not arguing that we make a change to go through and change all of our "< 38000" bits of code to "< '38000'" , I'm just saying that it looks to me like "Coerce the supplied val (typically a string) into an int or float if possible" arguably was doing the right thing for '01234' even if it wasn't expected. The fundamental issue was we didn't want "convert to int if possible" we wanted "always convert this obsid string to int".

[jeanconn]

I also wonder @taldcroft if you read my "I can see the need" as "I can't see the need". Because I can see the need for this code change and I think it is the right thing. I was mostly just commenting that obsids are annoying and the previous code isn't entirely to blame.

[taldcroft]

I added a note.

[javierggt]

I'm fine with this change. The name of the function bugs me a bit though. Instead of convert_to_int_float_str I would have expected something like convert_str_to_int_float