Adds a mixin (ImportJarsMixin) for the IvyImports task

Review Request #1783 — Created Feb. 16, 2015 and submitted

zundel
pants
zundel/import-jars-mixin
1115
1785
666e67b...
pants-reviews
jsirois, nhoward_tw, patricklaw

This replaces a label for the IvyImports task to find
targets that use the ivy-imports feature.

No functional changes: I gratutiously refactored a few names and moved some
common functionality into the mixin.

CI at https://travis-ci.org/pantsbuild/pants/builds/50921915

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
ZU
ZU
  1. 
      
  2. Got rid of this label. No one else should be using this.

  3. Common code with UnpackedJars.traversable_specs(), so I moved it to ImportJarsMixin.

  4. Again, more common code moved to the mixin.

  5. 
      
ZU
PA
  1. This worked out nicely!

  2. Is this a property of the target or of the task that operates on the target? If it's the latter, it could be rolled into a FingerprintStrategy subclass for that task.

    1. It is a property of the task, (an option set on the class). I see how that could work. The task is ProtobufGen, a subclass of codegen, so we'd have to refactor the codegen baseclass a bit to get it. I went ahead and added your comment to the TODO.

  3. While I've warmed up to "impure" mixins a bit, I'm still super leery of them having an __init__. If it's just for initializing "cache" attributes, I'd prefer to see the idiomatic style of making the cache attributes into class attributes just above the property that wraps the cached attribute, and then using them the same way.

    Or better yet, I'll go whip us up a cached_property utility for pants--something we've been seriously needing for a while now.

    1. Do you mean something like this?

        cached_imported_jars = {}
        @property
        def imported_jars(self):
          """:returns: the string specs of JarDependencies referenced by imported_jar_library_specs
          :rtype: list of string
          """
          if self not in self.cached_imported_jars:
            self.cached_imported_jars[self] =  JarLibrary.to_jar_dependencies(self.address,
                                                                  self.imported_jar_library_specs,
                                                                  self._build_graph)
          return self.cached_imported_jars[self]
      
    2. Like this:

      _imported_jars = None
      @property
      def imported_jars(self):
        if self._imported_jars is None:
          ... compute and set ...
        return self._imported_jars
      

      This relies on the behavior of first checking instance dicts for attributes on both setting and reading before moving up and checking class attributes. So you "default" the attribute by making it a class attribute, and then override it for a particular instance by setting it.

    3. After reading it a couple of times, I see how it works... You really think that is better than overriding __init__?

    4. I do--I know it's goofy, too. But it's idiomatic, and avoiding a mixin overriding __init__ I think outweighs it--especially since I will follow up with a less goofy decorator to encapsulate this pattern presently.

  4. This on the other hand is a very nice demonstration of what impure target mixins can buy us.

  5. 
      
ZU
JS
  1. 
      
  2. + 3rdparty/python:six
    1. I forgot to address this but fortunately, David Turner beat me to it:

      7169e9bd (David Turner   2015-02-17 13:44:21 -0500 27)     '3rdparty/python:six',
      
  3. +1 - I have the code for this tucked away as well.
  4. 
      
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews. commit d01cd1a2178d88b3960f2b85208f5c0fa6803800

Loading...