On Fri, Dec 20, 2013 at 11:41 AM, Helga Velroyen <[email protected]> wrote:
> > > > On Fri, Dec 20, 2013 at 10:09 AM, Hrvoje Ribicic <[email protected]> wrote: > >> As a helper or a warning to anyone extending the RAPI client, the >> client wrapper now warns of unused methods or method arguments. >> >> Signed-off-by: Hrvoje Ribicic <[email protected]> >> --- >> qa/rapi-workload.py | 102 >> +++++++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 74 insertions(+), 28 deletions(-) >> >> diff --git a/qa/rapi-workload.py b/qa/rapi-workload.py >> index b71d268..a7a35b3 100755 >> --- a/qa/rapi-workload.py >> +++ b/qa/rapi-workload.py >> @@ -26,8 +26,9 @@ >> # pylint: disable=C0103 >> # due to invalid name >> >> - >> +import inspect >> import sys >> +import types >> >> import ganeti.constants as constants >> from ganeti.rapi.client import GanetiApiError >> @@ -69,32 +70,6 @@ def MockMethod(*_args, **_kwargs): >> return None >> >> >> -def InvokerCreator(fn, name): >> - """ Returns an invoker function that will invoke the given function >> - with any arguments passed to the invoker at a later time, while >> - catching any specific non-fatal errors we would like to know more >> - about. >> - >> - @type fn arbitrary function >> - @param fn The function to invoke later. >> - @type name string >> - @param name The name of the function, for debugging purposes. >> - @rtype function >> - >> - """ >> - def decoratedFn(*args, **kwargs): >> - result = None >> - try: >> - print "Using method %s" % name >> - result = fn(*args, **kwargs) >> - except GanetiApiError as e: >> - print "RAPI error while performing function %s : %s" % \ >> - (name, str(e)) >> - return result >> - >> - return decoratedFn >> - >> - >> RAPI_USERNAME = "ganeti-qa" >> >> >> @@ -107,6 +82,45 @@ class GanetiRapiClientWrapper(object): >> self._client = qa_rapi.Setup(RAPI_USERNAME, >> qa_rapi.LookupRapiSecret(RAPI_USERNAME)) >> >> + self._method_invocations = {} >> + >> + def _RecordMethodInvocation(self, name, arg_dict): >> + """ Records the invocation of a C{GanetiRAPIClient} method, noting >> the >> + argument and the method names. >> + >> + """ >> + if name not in self._method_invocations: >> + self._method_invocations[name] = set() >> + >> + for named_arg in arg_dict: >> + self._method_invocations[name].add(named_arg) >> + >> + def _InvokerCreator(self, fn, name): >> + """ Returns an invoker function that will invoke the given function >> + with any arguments passed to the invoker at a later time, while >> + catching any specific non-fatal errors we would like to know more >> + about. >> + >> + @type fn arbitrary function >> + @param fn The function to invoke later. >> + @type name string >> + @param name The name of the function, for debugging purposes. >> + @rtype function >> + >> + """ >> + def decoratedFn(*args, **kwargs): >> + result = None >> + try: >> + print "Using method %s" % name >> + self._RecordMethodInvocation(name, kwargs) >> + result = fn(*args, **kwargs) >> + except GanetiApiError as e: >> + print "RAPI error while performing function %s : %s" % \ >> + (name, str(e)) >> + return result >> + >> + return decoratedFn >> + >> def __getattr__(self, attr): >> """ Fetches an attribute from the underlying client if necessary. >> >> @@ -116,12 +130,41 @@ class GanetiRapiClientWrapper(object): >> #guide, this will stop infinite loops in attribute fetches. >> if attr.startswith("_"): >> return self.__getattribute__(attr) >> + >> + # We also want to expose non-methods >> + if hasattr(self._client, attr) and \ >> + not isinstance(getattr(self._client, attr), types.MethodType): >> + return getattr(self._client, attr) >> + >> try: >> - return InvokerCreator(self._client.__getattribute__(attr), attr) >> + return self._InvokerCreator(self._client.__getattribute__(attr), >> attr) >> except AttributeError: >> print "Missing method %s; supplying mock method" % attr >> return MockMethod >> >> + def _OutputMethodInvocationDetails(self): >> + """ Attempts to output as much information as possible about the >> methods >> + that have and have not been invoked, including which arguments have >> not >> + been used. >> + >> + """ >> + print "\nMethod usage:\n" >> + for method in [n for n in dir(self._client) >> + if not n.startswith('_') and >> + isinstance(self.__getattr__(n), >> types.FunctionType)]: >> + if method not in self._method_invocations: >> + print "Method unused: %s" % method >> + else: >> + arg_spec = inspect.getargspec(getattr(self._client, method)) >> + default_args = [] > > + if arg_spec[3] is not None: >> > > For better readability, please write what the entry no 3 represents > (either in a comment or assign it to an expressive variable name first). > > ACK > > >> + default_args = arg_spec[0][-len(arg_spec[3]):] >> > > same for arg_spec[0] > ACK > > >> + used_arg_set = self._method_invocations[method] >> + unused_args = [arg for arg in default_args if arg not in >> used_arg_set] >> + if unused_args: >> + print "Method %s used, but arguments unused: %s" % \ >> + (method, ", ".join(unused_args)) >> + >> >> def Finish(client, fn, *args, **kwargs): >> """ When invoked with a job-starting RAPI client method, it passes >> along any >> @@ -660,6 +703,9 @@ def Main(): >> >> qa_node.TestNodeRemoveAll() >> >> + # pylint: disable=W0212 >> + client._OutputMethodInvocationDetails() >> + # pylint: enable=W0212 >> > > Add a comment about what the warning is. I find it tedious to always look > it up :) > Will do. > > >> >> if __name__ == "__main__": >> Main() >> -- >> 1.8.5.1 >> >> > LGTM, thanks > > -- > -- > Helga Velroyen | Software Engineer | [email protected] | > > Google Germany GmbH > Dienerstr. 12 > 80331 München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Thanks for the review, interdiff is: diff --git a/qa/rapi-workload.py b/qa/rapi-workload.py index dec38ae..e213327 100755 --- a/qa/rapi-workload.py +++ b/qa/rapi-workload.py @@ -155,10 +155,11 @@ class GanetiRapiClientWrapper(object): if method not in self._method_invocations: print "Method unused: %s" % method else: - arg_spec = inspect.getargspec(getattr(self._client, method)) + arg_spec, _, _, default_arg_spec = \ + inspect.getargspec(getattr(self._client, method)) default_args = [] - if arg_spec[3] is not None: - default_args = arg_spec[0][-len(arg_spec[3]):] + if default_arg_spec is not None: + default_args = arg_spec[-len(default_arg_spec):] used_arg_set = self._method_invocations[method] unused_args = [arg for arg in default_args if arg not in used_arg_set] if unused_args: @@ -700,6 +701,9 @@ def Main(): qa_node.TestNodeRemoveAll() + # The method invoked has the naming of the protected method, and pylint does + # not like this. Disabling the warning is healthier than explicitly adding and + # maintaining an exception for this method in the wrapper. # pylint: disable=W0212 client._OutputMethodInvocationDetails() # pylint: enable=W0212
