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