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

            pschella Pim Schellart [X] (Inactive) created issue -
            pschella Pim Schellart [X] (Inactive) made changes -
            Field Original Value New Value
            Epic Link DM-7717 [ 26925 ]
            rowen Russell Owen made changes -
            Assignee Russell Owen [ rowen ]
            rowen Russell Owen made changes -
            Sprint Alert Production S17 - 12 [ 305 ]
            Team Alert Production [ 10300 ]
            rowen Russell Owen made changes -
            Link This issue blocks DM-7057 [ DM-7057 ]
            pschella Pim Schellart [X] (Inactive) made changes -
            Link This issue blocks DM-8696 [ DM-8696 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue blocks DM-8434 [ DM-8434 ]
            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.
            rowen Russell Owen made changes -
            Reviewers Pim Schellart [ pschella ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            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.
            pschella Pim Schellart [X] (Inactive) made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-8716 [ DM-8716 ]
            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.
            rowen Russell Owen made changes -
            Status Reviewed [ 10101 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Story Points 2 6
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            tjenness Tim Jenness made changes -
            Component/s afw [ 10714 ]

              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