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

Use visitor pattern to clean up afw::table pybind11 wrappers

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      6
    • Sprint:
      DRP S17-3, DRP S17-4
    • Team:
      Data Release Production

      Description

      The way BaseRecord is wrapped in pybind11 needs some work:

      • get and __getitem__ should be wrapped the way they were with SWIG, which is simpler code than was used, and looks identical for the two methods (though that code uses $action so there are subtle differences). Better yet, make the methods identical if possible.
      • The wrappings should probably use C++ method _get, which needs to be added in the pybind11 wrapper file, rather than the specializations that use suffixes.
      • The C++ wrapper defines _getitem_ for each data type and the code is radically different than for _get_ + suffix. It would be nice if the differences could be eliminated.

      Similar issues apply to several other afw::table classes.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Finally ready for review. Since it's a weekend, I'm not expecting anyone to have time to look at it before we merge on Monday, but if you'd like to take a look, go ahead! I imagine on Monday morning we'll probably merge tickets like this first and then do a post-merge cleanup ticket to retroactively address any review comments.

            Show
            jbosch Jim Bosch added a comment - Finally ready for review. Since it's a weekend, I'm not expecting anyone to have time to look at it before we merge on Monday, but if you'd like to take a look, go ahead! I imagine on Monday morning we'll probably merge tickets like this first and then do a post-merge cleanup ticket to retroactively address any review comments.
            Hide
            rowen Russell Owen added a comment -

            I'm done with my review (since there are two other reviewers I will not mark it reviewed). Overall this looks great to me; a big improvement. I put some comments on github and wish I had more time to understand some of the changes, but don't want to hold up the merge.

            Show
            rowen Russell Owen added a comment - I'm done with my review (since there are two other reviewers I will not mark it reviewed). Overall this looks great to me; a big improvement. I put some comments on github and wish I had more time to understand some of the changes, but don't want to hold up the merge.
            Hide
            krzys Krzysztof Findeisen added a comment -

            I'm done with my review. I found some style issues and a few things I couldn't understand, but I agree that we shouldn't hold up the merge.

            Show
            krzys Krzysztof Findeisen added a comment - I'm done with my review. I found some style issues and a few things I couldn't understand, but I agree that we shouldn't hold up the merge.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Looks fine.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Looks fine.
            Hide
            jbosch Jim Bosch added a comment -

            Merged to tickets/DM-8467.

            Show
            jbosch Jim Bosch added a comment - Merged to tickets/ DM-8467 .

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                rowen Russell Owen
                Reviewers:
                Pim Schellart [X] (Inactive), Russell Owen
                Watchers:
                Fred Moolekamp, Jim Bosch, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel