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

rename CameraMapper.getEupsProductName() to getPackageName() and convert to abstract method

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: obs_base
    • Labels:
      None

      Description

      Per discussion on this PR related to DM-2636:
      https://github.com/lsst/daf_butlerUtils/pull/1#issuecomment-104785055

      The CameraMapper.getEupsProductName() should be renamed to getPackageName() and converted to an abstract method. This will eliminates a runtime, and thus "test time", dependency on EUPS. As part of the rename/conversion, all subclasses that are not already overriding getEupsProductName() will concurrently need to have getPackageName() implemented.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            I'm a bit confused. Tim: are you saying that something goes wrong if we use class variable packageName to get the package name, and otherwise use it exactly as it used now (including testing it in the base class's constructor) and get rid of the class method getPackageName()? Does it make pylint unhappy for some reason?

            I agree that using @property would be a bit safer but it doesn't work. K-T said yesterday he was not worried about buggy code accidentally changing the value of the class variable and I agree that it's unlikely to be a problem.

            Show
            rowen Russell Owen added a comment - - edited I'm a bit confused. Tim: are you saying that something goes wrong if we use class variable packageName to get the package name, and otherwise use it exactly as it used now (including testing it in the base class's constructor) and get rid of the class method getPackageName()? Does it make pylint unhappy for some reason? I agree that using @property would be a bit safer but it doesn't work. K-T said yesterday he was not worried about buggy code accidentally changing the value of the class variable and I agree that it's unlikely to be a problem.
            Hide
            tjenness Tim Jenness added a comment -

            The pylint comment refers to complaining if "_" methods or variables are used by different classes. Python doesn't have the notion of an internal variable that can be used by subclasses.

            I'm still trying to work out why this is a class method. The constructor trapping the package name not being set is irrelevant if people call the class method without instantiating an object. If this was an object method then getPackageName would serve no purpose and we could just delete it and use the attribute directly (maybe making it readonly). It seems that if we have a class variable we have no control over when people access it. Unless we switch to instance variables I'm inclined to just tag the class attribute with an underscore.

            Show
            tjenness Tim Jenness added a comment - The pylint comment refers to complaining if "_" methods or variables are used by different classes. Python doesn't have the notion of an internal variable that can be used by subclasses. I'm still trying to work out why this is a class method. The constructor trapping the package name not being set is irrelevant if people call the class method without instantiating an object. If this was an object method then getPackageName would serve no purpose and we could just delete it and use the attribute directly (maybe making it readonly). It seems that if we have a class variable we have no control over when people access it. Unless we switch to instance variables I'm inclined to just tag the class attribute with an underscore.
            Hide
            rowen Russell Owen added a comment - - edited

            It is not presently necessary for package name to be a class attribute, but logically it makes sense for it to be, and we may end up with a use case someday.

            However, there is no easy way to test that the subclass set it before the object is instantiated. K-T explicitly granted permission for the test to be deferred until then. There are alternatives, such as not testing it anywhere except a unit test. I object to that because I expect authors of new subclasses of CameraMapper will forget. The error will be caught eventually, but catching it earlier is nice if it's easy to do.

            Show
            rowen Russell Owen added a comment - - edited It is not presently necessary for package name to be a class attribute, but logically it makes sense for it to be, and we may end up with a use case someday. However, there is no easy way to test that the subclass set it before the object is instantiated. K-T explicitly granted permission for the test to be deferred until then. There are alternatives, such as not testing it anywhere except a unit test. I object to that because I expect authors of new subclasses of CameraMapper will forget. The error will be caught eventually, but catching it earlier is nice if it's easy to do.
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            The package name, ie. how the source code is organized, is class metadata which should be invariant for any instances.

            Show
            jhoblitt Joshua Hoblitt added a comment - The package name, ie. how the source code is organized, is class metadata which should be invariant for any instances.
            Hide
            rowen Russell Owen added a comment - - edited

            I agree that the package name is, by design, a constant for a given subclass of CameraMapper. The question is whether we need to enforce that const-ness. It is a pain to do in Python, so it's worth asking that question. I don't think we need to enforce const-ness and K-T said the same in the HipChat discussion (though it would carry more weight in this ticket).

            We don't enforce const-ness with any other Python class constants that I know of, such as Task.ConfigClass, and I am not aware of any problems that this has caused.

            Show
            rowen Russell Owen added a comment - - edited I agree that the package name is, by design, a constant for a given subclass of CameraMapper. The question is whether we need to enforce that const-ness. It is a pain to do in Python, so it's worth asking that question. I don't think we need to enforce const-ness and K-T said the same in the HipChat discussion (though it would carry more weight in this ticket). We don't enforce const-ness with any other Python class constants that I know of, such as Task.ConfigClass, and I am not aware of any problems that this has caused.

              People

              Assignee:
              jhoblitt Joshua Hoblitt
              Reporter:
              jhoblitt Joshua Hoblitt
              Reviewers:
              Kian-Tat Lim, Russell Owen, Tim Jenness
              Watchers:
              Frossie Economou, Joshua Hoblitt, Kian-Tat Lim, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.