Skip to content

Blitz static get query string#4993

Merged
jburel merged 12 commits into
ome:developfrom
will-moore:blitz_static_getQueryString
Jan 10, 2017
Merged

Blitz static get query string#4993
jburel merged 12 commits into
ome:developfrom
will-moore:blitz_static_getQueryString

Conversation

@will-moore
Copy link
Copy Markdown
Member

What this PR does

See discussion at #4950 (comment)
Since BlitzGateway wrapper_getQueryString() doesn't need to be an instance method, this is now a classmethod.

Testing this PR

Check that tests are green etc.

cc @aleksandra-tarkowska

@will-moore
Copy link
Copy Markdown
Member Author

Probably this PR is causing this error:

 File "/opt/hudson/workspace/WEB-DEV-integration-deploy/OMERO.py/lib/python/omero/gateway/__init__.py", line 774, in listParents
   for x in t])
TypeError: <lambda>() takes exactly 1 argument (2 given)

@chris-allan
Copy link
Copy Markdown
Member

chris-allan commented Dec 12, 2016

While I appreciate the work being done here what exactly are we intending to achieve structurally by changing this to a class method? Are we really so sure that we're never going to need anything from the instance to generate the query?

Edit: Moving things out of the bootstrap method also has some pretty serious downstream implications.

@will-moore
Copy link
Copy Markdown
Member Author

I think that _getQueryString() has to be callable without an instance of the object, since we need the query in order to retrieve any objects from the server. If you needed an instance to get the query, then we'd have a chicken and egg problem.
I think the main benefit from these changes is that we wouldn't have to instantiate a dummy object just to get the query. Currently we're doing:

wrapper()._getQueryString(opts)

This PR would allow us to get the query without creating a wrapper instance (in most cases) see ab9eba9

@chris-allan
Copy link
Copy Markdown
Member

A ha, makes sense. Also then makes complete sense to refactor away the bootstrap method. Do we have any cases where we are using it at all after the changes you've made in 1ae9fff? We also need to know a priori what the wrapper class type mapping is anyway correct? That is, wrapper() is providing us with no additional convenience or abstraction?

@will-moore
Copy link
Copy Markdown
Member Author

The error above in #4993 (comment) is caused by removal of __bstrap__() in WellSampleWrapper and can be fixed by adding back the __bstrap__ like this:

-    LINK_PARENT = lambda x: x
     LINK_CHILD = 'image'
 
+    def __bstrap__(self):
+        self.LINK_PARENT = lambda x: x

Or by providing E.g. a static method:

-    LINK_PARENT = lambda x: x
     LINK_CHILD = 'image'
+    
+    @staticmethod
+    def LINK_PARENT(parent):
+        return parent

I don't know if there's any reason why self.LINK_PARENT = lambda x: x should be better than this alternative?

The only other remaining use of __bstrap__() is in LaserWrapper

    def __bstrap__(self):
        super(_LaserWrapper, self).__bstrap__()
        self._attrs += (
            '#laserMedium',
            'frequencyMultiplication',
            'tuneable'....

@chris-allan
Copy link
Copy Markdown
Member

There's already a LINK_PARENT static method on BlitzObjectWrapper. It's not 100% clear to me exactly what the intention was overriding it this manner in the WellSampleWrapper. Certainly overriding the static method with another static method would work. We should probably put test cases in, if we don't have them already, to cover this functionality before messing around with it however; listParents() is super old code.

@will-moore
Copy link
Copy Markdown
Member Author

Yes, I'm already writing more tests for listParents(), since we only test it for PDI and it should work for a lot of others.

@will-moore
Copy link
Copy Markdown
Member Author

@chris-allan The BlitzObjectWrapper.LINK_PARENT(link) returns link.parent since that is how most objects are linked but there's no link between WellSample and Well, so WellSample.listParents() returns Wells rather than links.

@will-moore
Copy link
Copy Markdown
Member Author

@jburel jburel added the develop label Dec 13, 2016
@will-moore
Copy link
Copy Markdown
Member Author


def _getQueryString(self, opts=None):
@classmethod
def _getQueryString(klass, opts=None):
Copy link
Copy Markdown
Member

@atarkowska atarkowska Jan 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a historical (i.e. pre-PEP8) Josh/Carlos-style thing which should be dropped in favor of cls.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first argument to a class method is a reference to the class. As class is a "reserved" word in Python so we have to use something. klass is a classical style that we inherited from Java. cls is also fine and in line with the PEP8 suggestions as @joshmoore has already mentioned.

@jburel
Copy link
Copy Markdown
Member

jburel commented Jan 10, 2017

Reviewed with @chris-allan, merging.

@jburel jburel merged commit f1f83cb into ome:develop Jan 10, 2017
@jburel jburel added this to the 5.3.0 milestone Mar 29, 2017
@will-moore will-moore deleted the blitz_static_getQueryString branch February 18, 2019 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants