4:17 a.m.
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.
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.
Regards
Robert Kuska
rkuska(a)redhat.com
6:37 a.m.
New subject: Introduction+[PATCH]
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
6:47 a.m.
New subject: 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.
3:22 a.m.
New subject: Introduction+[PATCH]
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>
To: "Cool Other Package Repositories" <copr-devel(a)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(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
4:26 a.m.
New subject: Introduction+[PATCH]
On 06/27/2013 10:22 AM, Robert Kuska wrote:
I am sending attachment with improved patch. Comments and suggestions
about
doc_string of decorator are welcomed.
After quick chat on IRC and fixing few minor stuff - I committed it as
1269f2d.
Thanks.
--
Miroslav Suchy
Red Hat Systems Management Engineering
3955
days inactive
3956
days old
copr-devel@lists.fedorahosted.org
4 comments
3 participants
participants (3)
-
Bohuslav Kabrda -
Miroslav Suchý -
Robert Kuska