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@redhat.com To: "Cool Other Package Repositories" copr-devel@lists.fedorahosted.org 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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/copr-devel