Re: Introduction + [PATCH]

[Robert Kuska]

I would vote for the second approach as well because I find 'with' statements
simply useful.

I am sending attachment with improved patch. Comments and suggestions about 
doc_string of decorator are welcomed.

Regards
Robert Kuska






----- Original Message -----
From: "Bohuslav Kabrda" <bkabrda(a)redhat.com&gt;
To: "Cool Other Package Repositories" <copr-devel(a)lists.fedorahosted.org&gt;
Sent: Wednesday, June 26, 2013 1:47:32 PM
Subject: Re: Introduction + [PATCH]

----- Original Message -----
>  On 06/26/2013 11:17 AM, Robert Kuska wrote:
>  > Hi everyone,
>  >
>  > my name is Robert Kuska and I would like to start working
>  > on coprs_frontend.
>  >
>  > My plan is to start with test suites and documentation and
>  > after successful diving in also contribute and develope
>  > the frontend itself.
>  
>  Very nice. Welcome.
>   
Yes. I wanted to do this for a long time :)

>  
>  > I am sending my first patch proposal along with this
>  > introduction as an attachment (It's quite a long). I made a
>  > decorator as a replacement for context managers methods
>  > in tests. I hope it's fine.
>  
>  Why we use "with" on first place.
>  I always find it as ugly construct and tried to avoid that whole my life.
>   
Ugly is point of view :)

>  For example the code:
>           with self.tc as c:
>               with c.session_transaction() as s:
>                   s['openid'] = self.u3.openid_name
>  
>               self.db.session.add(self.u3)
>               r = c.get('/coprs/owned/{0}/'.format(self.u3.name))
>               assert 'No coprs...' in r.data
>  has 274 characters.
>  While the equivalent:
>           self.tc.session_transaction()['openid'] = self.u3.openid_name
>  
>           self.db.session.add(self.u3)
>           r = self.tc.get('/coprs/owned/{0}/'.format(self.u3.name))
>           assert 'No coprs...' in r.data
>  has only 214 character. And is IMO even more readable.
>   
But your code doesn't close the session transaction, which is precisely what
"with ..." is supposed to do. It's a standard Pythonic construct and is
definitely worth using precisely for this.

>  With your change:
>      @TransactionDecorator('u3')
>           self.db.session.add(self.u3)
>           r = self.c.get('/coprs/owned/{0}/'.format(self.u3.name))
>           assert 'No coprs...' in r.data
>  it has only 173. And I have to admit it is even little bit readable. But
>  mostly because you very clever get rid of the line:
>       self.tc.session_transaction()['openid'] = self.u3.openid_name
>  But you can do the same with method, which will be used like:
>       self._assign_openid_name(self.u3)
>  which is IMO readable.
>   
Yep, we could do that.

>  What worries me little bit is that it created variable, which is not
>  self-explanatary. I.e "s" instead of "session" or even
>  "session_transaction". And we-programmers tends to use these ugly short
>  variable in local block. So I can imagine, that it will be easily
>  accidentally overridden.
>  
>  What I would like is either
>    * get rid of "with" completly. And then there is no need to decorator
>  neither. Or: 
I disagree. Using "with" is right (see above). Moreover you would still need to
log user in every function anyway, so using the decorator keeps you from doing this and
it's done in one place.

>    * the decorator will define self-explanatory variable instead of
> "c"
>  and "s". And of course add doc_string to TransactionDecorator, which
>  will explain its behaviour.
>   
+1 here.

>  --
>  Miroslav Suchy
>  Red Hat Systems Management Engineering
>  _______________________________________________
>  copr-devel mailing list
>  copr-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/copr-devel
>   
-- 
Regards,
Bohuslav "Slavek" Kabrda.
_______________________________________________
copr-devel mailing list
copr-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/copr-devel