Introduction + [PATCH]

[Robert Kuska]

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
[e-mail]

[Miroslav Suchý]

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.

[Bohuslav Kabrda]

----- 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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/copr-devel

[Robert Kuska]

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" 
To: "Cool Other Package Repositories" 
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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/copr-devel

[Miroslav Suchý]

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.