Introduction + [PATCH]

Bohuslav Kabrda bkabrda at redhat.com
Wed Jun 26 11:47:32 UTC 2013


----- 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/copr-devel
> 

-- 
Regards,
Bohuslav "Slavek" Kabrda.


More information about the copr-devel mailing list