LGTM, thanks
On Fri, Dec 20, 2013 at 6:00 PM, Hrvoje Ribicic <[email protected]> wrote: > 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 > > > -- -- 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
