Review Request: conflicts and repoclosure tests minor refactoring

[Martin Krizek]

While studying conflicts and repoclosure tests I've done some minor code refactoring
that might be worth pushing to master, please review at origin/mkrizek:

$ git log origin/master..origin/mkrizek

Thanks,
Martin

[Kamil Paral]

>  While studying conflicts and repoclosure tests I've done some
> minor
>  code refactoring that might be worth pushing to master, please
>  review at origin/mkrizek:
>  
>  $ git log origin/master..origin/mkrizek
>  
>  Thanks,
>  Martin 
I wanted to respond "you can push small changes like these right away", but in
the end I have one concern:

>  self.detail.id = "%s-%s" % (name, kwargs['arch'])
> if name else baseurl 
This is extremely confusing, that really works? Usually I do use ternary operator, but in
this case it's not a good idea. I took me quite some time just to understand the
purpose and I'm still not sure whether it really works. I don't believe that
people usually remember which one of '%' and ternary operator has higher priority.
I don't.

[Martin Krizek]

----- Original Message -----
>  From: "Kamil Paral" <kparal(a)redhat.com&gt;
>  To: "AutoQA development" <autoqa-devel(a)lists.fedorahosted.org&gt;
>  Sent: Friday, March 2, 2012 4:06:54 PM
>  Subject: Re: Review Request: conflicts and repoclosure tests minor refactoring
>  
>  > While studying conflicts and repoclosure tests I've done some minor
>  > code refactoring that might be worth pushing to master, please
>  > review at origin/mkrizek:
>  > 
>  > $ git log origin/master..origin/mkrizek
>  > 
>  > Thanks,
>  > Martin
>  
>  I wanted to respond "you can push small changes like these right
>  away", but in the end I have one concern:
>  
>  > self.detail.id = "%s-%s" % (name, kwargs['arch']) if name else
>  > baseurl
>  
>  This is extremely confusing, that really works? Usually I do use
>  ternary operator, but in this case it's not a good idea. I took me
>  quite some time just to understand the purpose and I'm still not
>  sure whether it really works. I don't believe that people usually
>  remember which one of '%' and ternary operator has higher priority.
>  I don't. 
Sure it works [1]. Hm, is that really extremely confusing? I don't find 
it as such at all actually.

[1] http://docs.python.org/reference/expressions.html#conditional-expressions

[Kamil Paral]

>  > > self.detail.id = "%s-%s" % (name,
> kwargs['arch']) if name else
>  > > baseurl
>  
>  Sure it works [1]. Hm, is that really extremely confusing? I don't
>  find
>  it as such at all actually.
>  
>  [1]
>  http://docs.python.org/reference/expressions.html#conditional-expressions 
It might be confusing just for me, or you might not see it as the code author :) I'll
describe it better just for clarity sake.

The source of my confusion is which part of the line is related to the "if"
statement:

Option #1 (obviously the correct one):

self.detail.id = "%s-%s" % (name, kwargs['arch']) if name else baseurl
                 <------------------------------>

Option #2 (my first impression):

self.detail.id = "%s-%s" % (name, kwargs['arch']) if name else baseurl
                           <-------------------->

In this case it would crash, because '%' operator expects two arguments and
receives just one (baseurl).


But you are right, option #1 is correct. Still I don't recommend to write it this way,
as you see it's not that clear for everyone.

[Josef Skladanka]

+1 for kparal. Especially since on "first sight" I thought that the #2 is the
way it works,
and spent some time thinking about "how does this not yield exception", before
realising it :)

J.

----- Original Message -----
>  From: "Kamil Paral" <kparal(a)redhat.com&gt;
>  To: "AutoQA development" <autoqa-devel(a)lists.fedorahosted.org&gt;
>  Sent: Saturday, March 3, 2012 2:10:33 PM
>  Subject: Re: Review Request: conflicts and repoclosure tests minor refactoring
>  
>  > > > self.detail.id = "%s-%s" % (name, kwargs['arch']) if
> name else
>  > > > baseurl
>  > 
>  > Sure it works [1]. Hm, is that really extremely confusing? I don't
>  > find
>  > it as such at all actually.
>  > 
>  > [1]
>  > http://docs.python.org/reference/expressions.html#conditional-expressions
>  
>  It might be confusing just for me, or you might not see it as the
>  code author :) I'll describe it better just for clarity sake.
>  
>  The source of my confusion is which part of the line is related to
>  the "if" statement:
>  
>  Option #1 (obviously the correct one):
>  
>  self.detail.id = "%s-%s" % (name, kwargs['arch']) if name else
>  baseurl
>                   <------------------------------>
>  
>  Option #2 (my first impression):
>  
>  self.detail.id = "%s-%s" % (name, kwargs['arch']) if name else
>  baseurl
>                             <-------------------->
>  
>  In this case it would crash, because '%' operator expects two
>  arguments and receives just one (baseurl).
>  
>  
>  But you are right, option #1 is correct. Still I don't recommend to
>  write it this way, as you see it's not that clear for everyone.
>  _______________________________________________
>  autoqa-devel mailing list
>  autoqa-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/autoqa-devel
>