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

Add getScalar and getArray methods to PropertySet and PropertyList and prefer them

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: daf_base
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      AP S18-6, AP F18-1
    • Team:
      Alert Production

      Description

      Implement RFC-434 by adding getScalar(name) and getArray(name) methods to PropertySet and PropertyList and possibly adding a deprecation warning to get.

        Attachments

          Issue Links

            Activity

            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Link This issue is triggered by RFC-434 [ RFC-434 ]
            swinbank John Swinbank made changes -
            Team Data Access and Database [ 10204 ]
            rowen Russell Owen made changes -
            Assignee Russell Owen [ rowen ]
            rowen Russell Owen made changes -
            Risk Score 0
            Hide
            rowen Russell Owen added a comment - - edited

            getArray and getScalar primarily make sense if the element type is a scalar (numeric or string), rather than a PropertySet or PropertyList. I don't see how getArray can return an array and getScalar can return a scalar unless the element type is a scalar. So I have made both methods raise TypeError if the element type is not a scalar.

            Thus get is still a useful method and cannot be deprecated.

            Show
            rowen Russell Owen added a comment - - edited getArray and getScalar primarily make sense if the element type is a scalar (numeric or string), rather than a PropertySet or PropertyList . I don't see how getArray can return an array and getScalar can return a scalar unless the element type is a scalar. So I have made both methods raise TypeError if the element type is not a scalar. Thus get is still a useful method and cannot be deprecated.
            rowen Russell Owen made changes -
            Epic Link DM-10068 [ 31628 ]
            rowen Russell Owen made changes -
            Sprint AP S18-6 [ 686 ]
            Team Data Access and Database [ 10204 ] Alert Production [ 10300 ]
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            rowen Russell Owen added a comment -

            I also took the opportunity to remove python 2 support.

            Show
            rowen Russell Owen added a comment - I also took the opportunity to remove python 2 support.
            rowen Russell Owen made changes -
            Reviewers Kian-Tat Lim [ ktl ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            ktl Kian-Tat Lim added a comment -

            The code is acceptable as is, but I think some could be condensed, either by deprecating get() as foreseen or by reimplementing the new methods in terms of get(). Also one minor doc issue (the penalty for fixing is to have to fix further...).

            Show
            ktl Kian-Tat Lim added a comment - The code is acceptable as is, but I think some could be condensed, either by deprecating get() as foreseen or by reimplementing the new methods in terms of get() . Also one minor doc issue (the penalty for fixing is to have to fix further...).
            ktl Kian-Tat Lim made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rowen Russell Owen added a comment -

            I have updated the code as follows:

            • Made asScalar capable of returning PropertySet and other such types. asArray still raises for those.
            • _propertyContainerGet now does all the work, with a new argument enum that can be ARRAY, SCALAR or AUTO
            • Deleted the asArray argument to get. The only use I found in our stack was incorrect (the argument was being treated as a default value; fixed on DM-14770). This makes the new API easier to understand because get(name, asArray=True) could handle PropertySet and such, and getArray cannot.
            • Made get issue a DeprecationWarning. To do this I rewrote some existing internal code to call _propertyContainerGet so it would continue to work unchanged and without issuing a warning. I also added callGet functions to two unit tests that verify that the warning is issued; I would rather have written a context manager for this job but decided it was not worth the time to figure out how to do that. I can centralize that if desired, e.g. in a new testUtils.py module, but it's small enough I chose to duplicate the code.
            Show
            rowen Russell Owen added a comment - I have updated the code as follows: Made asScalar capable of returning PropertySet and other such types. asArray still raises for those. _propertyContainerGet now does all the work, with a new argument enum that can be ARRAY , SCALAR or AUTO Deleted the asArray argument to get . The only use I found in our stack was incorrect (the argument was being treated as a default value; fixed on DM-14770 ). This makes the new API easier to understand because get(name, asArray=True) could handle PropertySet and such, and getArray cannot. Made get issue a DeprecationWarning . To do this I rewrote some existing internal code to call _propertyContainerGet so it would continue to work unchanged and without issuing a warning. I also added callGet functions to two unit tests that verify that the warning is issued; I would rather have written a context manager for this job but decided it was not worth the time to figure out how to do that. I can centralize that if desired, e.g. in a new testUtils.py module, but it's small enough I chose to duplicate the code.
            Hide
            rowen Russell Owen added a comment -

            Please have another look. Warning: I have not done any rebasing yet; I wanted to be sure this is an improvement before doing that.

            Show
            rowen Russell Owen added a comment - Please have another look. Warning: I have not done any rebasing yet; I wanted to be sure this is an improvement before doing that.
            rowen Russell Owen made changes -
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Sprint AP S18-6 [ 686 ] AP S18-6, AP F18-1 [ 686, 746 ]
            rowen Russell Owen made changes -
            Story Points 2 4
            Hide
            ktl Kian-Tat Lim added a comment -

            Looks fine (modulo Tim's comment about the tests).

            Show
            ktl Kian-Tat Lim added a comment - Looks fine (modulo Tim's comment about the tests).
            ktl Kian-Tat Lim made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-14770 [ DM-14770 ]
            swinbank John Swinbank made changes -
            Epic Link DM-10068 [ 31628 ] DM-14447 [ 80385 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Kian-Tat Lim
              Watchers:
              Kian-Tat Lim, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.