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

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

    XMLWordPrintable

Details

    • Improvement
    • Status: Done
    • Resolution: Done
    • None
    • afw
    • 6
    • DRP S17-3, DRP S17-4
    • 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

            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.

            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.

            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.

            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.

            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.

            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.

            Looks fine.

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

            Merged to tickets/DM-8467.

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

            People

              jbosch Jim Bosch
              rowen Russell Owen
              Pim Schellart [X] (Inactive), Russell Owen
              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:

                Jenkins

                  No builds found.