Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: pex_policy
-
Labels:
-
Story Points:1
-
Epic Link:
-
Sprint:Alert Production S17 - 12
-
Team:Alert Production
Description
The method names(bool topLevelOnly = false) in pex::policy::Policy does not use its argument, always behaving as if topLevelOnly were true. It appears that this bug went undetected because the swig wrapper for names(list<string> &, bool, bool) masked names(bool), preventing it from ever being called from Python code.
I propose that the implementation of names(bool) be changed to share code with names(list<string> &, bool, bool), both fixing the immediate problem and preventing inconsistent behavior from calling the wrong overload in the future.
Attachments
Issue Links
- blocks
-
DM-8423 Wrap obs_base (was daf_butlerUtils) with pybind11
- Done
Activity
An excellent improvement to the code.
My one minor discomfort is that the existing test for policyNames seems weak, and you changed the implementation of that method. However, the existing test may be good enough and your new implementation is a significant improvement (and almost certainly correct by inspection).
I'm also a bit surprised you duplicated a test in C++ and Python. I realize that names(list, bool, bool) is not wrapped for Python and so much be tested in C++ , but why test names(bool) in both? However, I realize opinions may differ on this, and having written the C++ test it would be a waste to discard it.
I guess my reasoning was that, given that I know there are/were problems with how the code was wrapped, it would be useful to know if the method behaves differently in one language than the other – information that could be used to diagnose future problems (indeed, prior to the implementation change, the C++ tests failed but the Python ones did not).
Krzysztof Findeisen
added a comment - I guess my reasoning was that, given that I know there are/were problems with how the code was wrapped, it would be useful to know if the method behaves differently in one language than the other – information that could be used to diagnose future problems (indeed, prior to the implementation change, the C++ tests failed but the Python ones did not). 
Hi Russell Owen, please review this fix.