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

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

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      6
    • Epic Link:
    • 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

          blocks

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-9100 Merge pybind11 branches to master

          • Done
          is blocked by

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-8805 Metaclass for wrapped-template ABCs and class extension decorators

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-9099 Begin cleanup of pybind11 branch

          • Done
          relates to

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-9018 Investigate downcasting of Table and Record types in pybind11

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-8674 Wrap testSourceTable with pybind11

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-6782 afw table string keyword access is much slower than asKey access

          • Invalid

            Activity

            Ascending order - Click to sort in descending order
            4 older comments
            Hide
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            pschella Pim Schellart [X] (Inactive) added a comment -

            Looks fine.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Looks fine.
            Hide
            Permalink
            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:

                  Jenkins

                  No builds found.