>
> On Wed, 2018-05-16 at 16:35 +0100, Frediano Ziglio wrote:
> > Code that handled client and server messages check was the same, just
> > changed some variable names.
> > Instead use a class to store same information and reuse the code.
> > This allows easier extension of the 2 path of code.
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > python_modules/ptypes.py | 68 +++++++++++++++++-----------------------
> > 1 file changed, 29 insertions(+), 39 deletions(-)
> >
> > Changes since v1:
> > - avoid __dict__ usage.
> >
> > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > index 0f6d8d6..fd9b1d0 100644
> > --- a/python_modules/ptypes.py
> > +++ b/python_modules/ptypes.py
> > @@ -1021,56 +1021,46 @@ class ChannelType(Type):
> > return self.server_messages_byname[name]
> >
> > def resolve(self):
> > - if self.base != None:
> > + class MessagesInfo:
> > + def __init__(self, is_server, messages = [], messages_byname =
> > {}):
>
> PEP8 says no spaces around '=' in function aruments.
>
I'll do
> > + self.is_server = is_server
> > + self.messages = messages[:]
>
> If the purpose of [:] is to make a copy, I'd vouch for calling .copy()
> to make it more explicit...
>
Does not work on Python 2, only Python 3, see:
>>> x=[]
>>> x.copy()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'list' object has no attribute 'copy'
> > + self.messages_byname = messages_byname.copy()
> > + self.count = 1
> > +
> > + if self.base == None:
>
> Strictly speaking, you should be using "self.base is None" instead of
> the == operator...
>
I'll do it, previous code was doing a != so I kept same syntax
but "is None" is surely more readable.
> > + server_info = MessagesInfo(True)
> > + client_info = MessagesInfo(False)
> > + else:
> > self.base = self.base.resolve()
> >
> > - server_messages = self.base.server_messages[:]
> > - server_messages_byname =
> > self.base.server_messages_byname.copy()
> > - client_messages = self.base.client_messages[:]
> > - client_messages_byname =
> > self.base.client_messages_byname.copy()
> > + server_info = MessagesInfo(True, self.base.server_messages,
> > + self.base.server_messages_byname)
> > + client_info = MessagesInfo(False, self.base.client_messages,
> > + self.base.client_messages_byname)
>
> It would probably be cleaner to copy the MessagesInfo class by either
> implementing a MessagesInfo.copy() method that copies the whole object
> or by using the copy module:
>
> import copy
>
> copied_info = copy.deepcopy(info_to_copy)
>
> (at least I think this should work :))
>
I don't want to do a deep copy. Why I should?
> You'd also have to make MessagesInfo object a member of this class,
> instead of pulling the list and dict out of it at the end of the
> patch...
>
What do you mean? Are you asking me to refactor all code?
> Just a suggestion though, I might have missed something.
>
> Cheers,
> Lukas
>
> > # Set default member_name, FooChannel -> foo
> > self.member_name = self.name[:-7].lower()
> > - else:
> > - server_messages = []
> > - server_messages_byname = {}
> > - client_messages = []
> > - client_messages_byname = {}
> >
> > - server_count = 1
> > - client_count = 1
> > -
> > - server = True
> > + info = server_info
> > for m in self.members:
> > if m == "server":
> > - server = True
> > + info = server_info
> > elif m == "client":
> > - server = False
> > - elif server:
> > - m.is_server = True
> > - m = m.resolve(self)
> > - if m.value:
> > - server_count = m.value + 1
> > - else:
> > - m.value = server_count
> > - server_count = server_count + 1
> > - server_messages.append(m)
> > - server_messages_byname[m.name] = m
> > + info = client_info
> > else:
> > - m.is_server = False
> > + m.is_server = info.is_server
> > m = m.resolve(self)
> > - if m.value:
> > - client_count = m.value + 1
> > - else:
> > - m.value = client_count
> > - client_count = client_count + 1
> > - client_messages.append(m)
> > - client_messages_byname[m.name] = m
> > -
> > - self.server_messages = server_messages
> > - self.server_messages_byname = server_messages_byname
> > - self.client_messages = client_messages
> > - self.client_messages_byname = client_messages_byname
> > + if not m.value:
> > + m.value = info.count
> > + info.count = m.value + 1
> > + info.messages.append(m)
> > + info.messages_byname[m.name] = m
> > +
> > + self.server_messages = server_info.messages
> > + self.server_messages_byname = server_info.messages_byname
> > + self.client_messages = client_info.messages
> > + self.client_messages_byname = client_info.messages_byname
> >
> > return self
> >
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel