# Add getScalar and getArray methods to PropertySet and PropertyList and prefer them

XMLWordPrintable

## Details

• Type: RFC
• Status: Implemented
• Resolution: Done
• Component/s:
• Labels:
None

## Description

In Python PropertySet.get(name) will return a scalar if there is only one element, and an array if there is more than one element. Similarly for PropertyList. I think this encourages unsafe code. When expecting a scalar it is far too easy to expect there will only be one element and not check. Similarly, when expecting an array, one may be surprised to get a scalar if the array happens to only have one element.

I suggest we add two methods to PropertySet and PropertyList in Python:

• getScalar(name): return the last value of the array, like C++ get<T>
• getArray(name): return the data as an array, even if it only has one element. This is like C++ getArray<T>.

I further suggest that we deprecate get and use these new methods for all new code. It avoids obvious bugs and makes the intent clearer. Here are some before and after examples:

 scalar = metadata.get("ascalar") # unsafe; breaks if more than one value scalar = metadata.get("ascalar", asArray=True)[-1] # safe and clear but ugly scalar = metadata.get("ascalar", True)[-1] # safe but unclear scalar = metadata.getScalar("ascalar")   array = metadata.get("anarray") # unsafe; breaks if only one value array = metadata.get("anarray", asArray=True) # safe and clear array = metadata.get("anarray", True) # safe but unclear array = metadata.getArray("anarray") 

Note that the safe old style examples use the flag asArray. This is an optional argument to get that I didn't know existed until today. It is undocumented. I'm curious how many folks even knew about it, and are confident they know what it does.

## Activity

Hide
Russell Owen added a comment -

Other than a few comments in Slack I've seen no discussion. Robert Lupton said something about FITS that I can't find right now.

I did not not intended this RFC to be about FITS. I proposed it for two main reasons:

• It is safer and more predictable than the current technique of getting array if > 1 value and a scaler if 1 value is neither.
• It makes the Python more closely match the C++

One issue is what getScalar should do if there is more than one value. This RFC proposes to silently return the last value, just like C++ get<type> ("most recent wins"). Another option would be to raise an exception. I prefer the former but am open to the latter, or having a doRaise argument, if others feel strongly enough about it.

Show
Russell Owen added a comment - Other than a few comments in Slack I've seen no discussion. Robert Lupton said something about FITS that I can't find right now. I did not not intended this RFC to be about FITS. I proposed it for two main reasons: It is safer and more predictable than the current technique of getting array if > 1 value and a scaler if 1 value is neither. It makes the Python more closely match the C++ One issue is what getScalar should do if there is more than one value. This RFC proposes to silently return the last value, just like C++ get<type> ("most recent wins"). Another option would be to raise an exception. I prefer the former but am open to the latter, or having a doRaise argument, if others feel strongly enough about it.
Hide
Tim Jenness added a comment -

I'd like Kian-Tat Lim to comment on this. It looks good to me. When you say "deprecate" do you mean in the sense of "we'd rather you not use it" or in the sense that the python API will issue a warning?

Show
Tim Jenness added a comment - I'd like Kian-Tat Lim to comment on this. It looks good to me. When you say "deprecate" do you mean in the sense of "we'd rather you not use it" or in the sense that the python API will issue a warning?
Hide
Russell Owen added a comment -

I think a deprecation warning is only appropriate once existing code has been modified to use the new API. When we want to schedule that work is an open question.

Show
Russell Owen added a comment - I think a deprecation warning is only appropriate once existing code has been modified to use the new API. When we want to schedule that work is an open question.
Hide
Kian-Tat Lim added a comment -

I'm fine with this.  One question I just thought of: is there any value to having the C++ get change to getScalar as well?

Show
Kian-Tat Lim added a comment - I'm fine with this.  One question I just thought of: is there any value to having the C++ get change to getScalar as well?
Hide
Russell Owen added a comment - - edited

Kian-Tat Lim good point. It would improve consistency and clarity to rename the C++ method to getScalar as well, even though C++ requires an explicit template parameter (e.g. getScalar<int>). I'll give folks a bit more time to complain before adopting that change.

Show
Russell Owen added a comment - - edited Kian-Tat Lim good point. It would improve consistency and clarity to rename the C++ method to getScalar as well, even though C++ requires an explicit template parameter (e.g. getScalar<int> ). I'll give folks a bit more time to complain before adopting that change.
Hide
Russell Owen added a comment -

Note that I don't propose renaming getAsDouble and such. They have the same behavior in C++ and Python: return the final value, cast as requested.

After thinking about it more I am not keen to change get<type> to getScalar<type> in C+. The C+ API is already fine. It is true that it is a small improvement to consistency between the C++ and Python, but they don't match either way and I don't think it is worth the effort. It could also be done at a later point if desired.

Show
Russell Owen added a comment - Note that I don't propose renaming getAsDouble and such. They have the same behavior in C++ and Python: return the final value, cast as requested. After thinking about it more I am not keen to change get<type> to getScalar<type> in C+ . The C + API is already fine. It is true that it is a small improvement to consistency between the C++ and Python, but they don't match either way and I don't think it is worth the effort. It could also be done at a later point if desired.
Hide
Kian-Tat Lim added a comment -

After thinking about it more I am not keen to change get<type> to getScalar<type> in C++.

OK with me; it was just a thought.

Show
Kian-Tat Lim added a comment - After thinking about it more I am not keen to change get<type> to getScalar<type> in C++. OK with me; it was just a thought.
Hide
Russell Owen added a comment -

• Add PropertySet.getScalar(name) which returns the final value, like get<type> in C++
• Document that get is deprecated in favor of getScalar and the existing getArray
• Once code has been updated to use getScalar add a deprecation warning to get
Show
Russell Owen added a comment - Adopted as follows: Add PropertySet.getScalar(name) which returns the final value, like get<type> in C++ Document that get is deprecated in favor of getScalar and the existing getArray Once code has been updated to use getScalar add a deprecation warning to get

## People

• Assignee:
Russell Owen
Reporter:
Russell Owen
Watchers:
John Swinbank, Kian-Tat Lim, Russell Owen, Tim Jenness