Introduction + [PATCH]

Miroslav Suchý msuchy at redhat.com
Wed Jun 26 11:37:24 UTC 2013


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.


> 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.

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.

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.

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:
  * 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.

-- 
Miroslav Suchy
Red Hat Systems Management Engineering


More information about the copr-devel mailing list