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

Wrap testSourceTable with pybind11

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Story Points:
      6
    • Sprint:
      Alert Production S17 - 12
    • Team:
      Alert Production

      Description

      Wrapping testSourceTable.py with pybind11 was part of DM-6297 but didn't really fit there.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Work includes:

            • Adding Record and Table attributes to all catalogs (as SWIG had before; this is needed for casting). Unfortunately this is done manually and is easy to forget. I could add it to declareCatalog but am not sure how to prevent getting the two attributes swapped (the record and table types would have to be template parameters and are treated equally). I decided explicit is safer.
            • Added support for casting records, tables and catalogs. I implemented SWIG's _cast method as _castFrom because I found it confusing that _cast and cast work in opposite directions and I wanted to make it easier for some future programmer.
            • Re-added the original support for caching column views. I feel the code (copied from the SWIG) is rather complicated and fragile and that we would be better off implementing this C++ (if we must have it) so I will file a ticket.
            • Standardized the table wrapper files so they all look the same and work the same way. This did involve moving a lot of code into functions in an anonymous namespace, so it will look like a bigger change than it actually is. I hope the reviewer is comfortable with the chosen technique. I had several to choose from amongst the various wrapper files and picked the one I found most readable.
            Show
            rowen Russell Owen added a comment - Work includes: Adding Record and Table attributes to all catalogs (as SWIG had before; this is needed for casting). Unfortunately this is done manually and is easy to forget. I could add it to declareCatalog but am not sure how to prevent getting the two attributes swapped (the record and table types would have to be template parameters and are treated equally). I decided explicit is safer. Added support for casting records, tables and catalogs. I implemented SWIG's _cast method as _castFrom because I found it confusing that _cast and cast work in opposite directions and I wanted to make it easier for some future programmer. Re-added the original support for caching column views. I feel the code (copied from the SWIG) is rather complicated and fragile and that we would be better off implementing this C++ (if we must have it) so I will file a ticket. Standardized the table wrapper files so they all look the same and work the same way. This did involve moving a lot of code into functions in an anonymous namespace, so it will look like a bigger change than it actually is. I hope the reviewer is comfortable with the chosen technique. I had several to choose from amongst the various wrapper files and picked the one I found most readable.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Looks fine. Minor suggestions on PR.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Looks fine. Minor suggestions on PR.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the helpful review.

            I adopted most of your suggestions.

            Show
            rowen Russell Owen added a comment - Thank you for the helpful review. I adopted most of your suggestions.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                pschella Pim Schellart [X] (Inactive)
                Reviewers:
                Pim Schellart [X] (Inactive)
                Watchers:
                Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel