Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-8654

Policy::names(bool) ignores its argument

    XMLWordPrintable

    Details

    • 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

            Activity

            No builds found.
            krzys Krzysztof Findeisen created issue -
            krzys Krzysztof Findeisen made changes -
            Field Original Value New Value
            Epic Link DM-7362 [ 26448 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8423 [ DM-8423 ]
            krzys Krzysztof Findeisen made changes -
            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(std::list<std::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(std::list<std::string> &, bool, bool)}}, both fixing the immediate problem and preventing inconsistent behavior from calling the wrong overload in the future.
            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.
            krzys Krzysztof Findeisen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            krzys Krzysztof Findeisen made changes -
            Story Points 2 1
            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Russell Owen, please review this fix.

            Show
            krzys Krzysztof Findeisen added a comment - Hi Russell Owen , please review this fix.
            krzys Krzysztof Findeisen made changes -
            Reviewers Russell Owen [ rowen ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            rowen Russell Owen added a comment -

            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.

            Show
            rowen Russell Owen added a comment - 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.
            rowen Russell Owen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            krzys 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).

            Show
            krzys 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).
            krzys Krzysztof Findeisen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            krughoff Simon Krughoff made changes -
            Epic Link DM-7362 [ 26448 ] DM-8472 [ 28104 ]

              People

              Assignee:
              krzys Krzysztof Findeisen
              Reporter:
              krzys Krzysztof Findeisen
              Reviewers:
              Russell Owen
              Watchers:
              Krzysztof Findeisen, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.