Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: daf_base
-
Labels:
-
Story Points:0.25
-
Epic Link:
-
Sprint:Alert Production S17 - 4
-
Team:Alert Production
Description
foo = PropertySet()
|
len(foo)
|
throws an exception in Python. The error is in propertyContainer/propertyContainerContinued.py which defines __len__ to return self.size(), but size is not a method on PropertySet.
Unfortunately len(propertyList) is not well defined (nor is it used, or we'd have caught this error before). In C++ begin and end iterate over unique names. But in Python we often view a PropertyList as a representation of FITS header cards, and viewed that way a user would expect len(propertyList) to return every entry. Kian-Tat Lim and I agree that since __len__ is ambiguous, it should be removed. Use named methods instead:
- To obtain the number of unique names use nameCount()
- There is no named method to obtain the number of entries. len(propertyList.toList()) will do, but is not very efficient. However, adding a simpler named method is out of scope for this ticket.
Attachments
Activity
There's one commit message that does not seem to match the changes that were in the commit, I made a note in the review. Otherwise everything looks good. Nice cleanup.
I recommend leaving some description of the issue with __len__ in the code itself. The description here in the ticket is very informative and I think it would be useful to have in the code for the next person who runs across this issue. Otherwise everything looks a-ok.
Thank you for the quick review. I found two errors in the commit history and cleaned them up. I'll merge after Jenkins builds.
I added a note to the pybind11 wrapper for PropertyList explaining why __len__ is not wrapped.
The primary change is trivial: remove PropertyList.__len__.
I also performed a cleanup pass on the python – autopep8, modernize the imports and the boilerplate (though I retained the listing of copyright in the boilerplate because I'm not sure we've really moved to having a separate copyright file) – all as separate commits.