Re: Revert 165f44aa?

[Claude Paroz]

Le dimanche 22 septembre 2013 14:24:20 UTC+2, Aymeric Augustin a écrit :
>
> I reverse-applied 165f44aa and committed a subset of the changes. 
>

I'm fine with these changes. Thanks for your work.

Claude

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Revert 165f44aa?

[Aymeric Augustin]

I reverse-applied 165f44aa and committed a subset of the changes.

For the record, here are the changes I did *not* commit.

-- 
Aymeric.


On 22 sept. 2013, at 09:09, Aymeric Augustin 
 wrote:

> Thanks everyone for your feedback.
> 
> We have a clear consensus for not merging consecutive `with` statements, 
> except in simple cases such as Russell's example.
> 
> I'll review each chunk of the patch and perform a partial revert.
> 
> -- 
> Aymeric.
> 



165f44aa-not-reverted.diff
Description: Binary data
-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Revert 165f44aa?

[Aymeric Augustin]

Thanks everyone for your feedback.

We have a clear consensus for not merging consecutive `with` statements, except 
in simple cases such as Russell's example.

I'll review each chunk of the patch and perform a partial revert.

-- 
Aymeric.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Revert 165f44aa?

[Russell Keith-Magee]

On Sun, Sep 22, 2013 at 5:16 AM, Florian Apolloner wrote:

>
>
> On Saturday, September 21, 2013 7:50:34 PM UTC+2, Aymeric Augustin wrote:
>>
>> But whenever the with statement spills over two lines, which happens in a
>> majority of cases, I find it worse than two with statements. It's
>> especially bad in the transactions tests — I worked in this area today.
>>
>
> Agreed; as a groundrule I'd say:
>  * If you need backslashes keep them separate
>  * If one is a test assertion keep them separate
>

I think we're all in agreement (or should be!) that backslashes are the
devil, and should be killed with fire. :-)

But I think the case is a little broader than the second one -- it's not
just test assertions, it's when the context is stateful (for want of a much
better word to describe the point).

Some context managers are just setting a local variable -- e.g., using
open() as a context manager. Setting 2+ variables in one statement is a bit
of a value judgement -- you're balancing reading multiple statements in a
single line against the 'cost' of multiple indentations. For example, I
have no problem seeing how:

with self.settings(USE_I18N=True), translation.override('pt-br',
deactivate=True):

could be seen as easier to read as a single statement, with single
indentation.

However, other context managers are heavily stateful, and the indentation
helps to describe that state. A test assertion really does "apply" to
everything "in" it. A transaction context manager wraps a very specific
block of code. In these cases, I know the language is unambiguous, but
there's value in using indentation to reinforce the physical structure,
IMHO.

Yours,
Russ Magee %-)

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Revert 165f44aa?

[Kevin Christopher Henry]

To summarize the possible approaches here:

1) Combine multiple with statements into one wherever possible. This seems 
to be the approach of the commit in question.
2) Group with statements based on whether they logically belong together, 
regardless of line length. This will involve backslashes, but note that PEP 
8 specifically blesses this use case: "Long, multiple with-statements 
cannot use implicit continuation, so backslashes are acceptable".
3) Group together logically related with statements unless that would 
involve backslashes, in which case use separate with statements. I think 
this is Marc and Florian's suggestion. The problem here is that the 
grouping of with statements depends on both logical and lexical 
considerations, which seems somewhat less readable to me.
4) Revert the commit and stick with one with statement per line.

Personally, I prefer 4 or 2.

Kevin


On Saturday, September 21, 2013 5:16:04 PM UTC-4, Florian Apolloner wrote:
>
>
>
> On Saturday, September 21, 2013 7:50:34 PM UTC+2, Aymeric Augustin wrote:
>>
>> But whenever the with statement spills over two lines, which happens in a 
>> majority of cases, I find it worse than two with statements. It's 
>> especially bad in the transactions tests — I worked in this area today. 
>>
>
> Agreed; as a groundrule I'd say:
>  * If you need backslashes keep them separate
>  * If one is a test assertion keep them separate
>
> You have a point about consistency, but as long as they are readable I can 
> live with having to versions…
>
> Florian
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Revert 165f44aa?

[Florian Apolloner]

On Saturday, September 21, 2013 7:50:34 PM UTC+2, Aymeric Augustin wrote:
>
> But whenever the with statement spills over two lines, which happens in a 
> majority of cases, I find it worse than two with statements. It's 
> especially bad in the transactions tests — I worked in this area today. 
>

Agreed; as a groundrule I'd say:
 * If you need backslashes keep them separate
 * If one is a test assertion keep them separate

You have a point about consistency, but as long as they are readable I can 
live with having to versions…

Florian

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Revert 165f44aa?

[Marc Tamlyn]

Burn the backslashes!

On a more serious note, I think these need to be taken on a case by case
basis. Certainly unhelpful it test code though.
On 21 Sep 2013 18:50, "Aymeric Augustin" 
wrote:

> Hello,
>
> I think we went overboard in this commit:
> https://github.com/django/django/commit/165f44aa
>
> Specifically, I consider it's a regression in three areas.
>
>
> 1) Readability
>
> After the patch, indentation no longer matches the logical structure of
> the program. For example:
>
> -with self.settings(USE_L10N=True, USE_THOUSAND_SEPARATOR=False):
> -with translation.override('en'):
> -self.humanize_tester(test_list, result_list, 'intcomma')
> +with self.settings(USE_L10N=True, USE_THOUSAND_SEPARATOR=False), \
> +translation.override('en'):
> +self.humanize_tester(test_list, result_list, 'intcomma')
>
> The new version is longer than the old one (by two characters) and
> visually harder to parse.
>
>
> 2) Readability (bis)
>
> After the patch, distinct operations end up on the same line, obfuscating
> the intent of the code. For example :
>
> -with six.assertRaisesRegex(self, Exception, "Oops"):
> -with transaction.atomic():
> -Reporter.objects.create(first_name="Haddock")
> -raise Exception("Oops, that's his last name")
> +with six.assertRaisesRegex(self, Exception, "Oops"),
> transaction.atomic():
> +Reporter.objects.create(first_name="Haddock")
> +raise Exception("Oops, that's his last name")
>
> In the old version, the first line is a test assertion and the second line
> is the code exercised by the test. I find it confusing to put both on the
> same line.
>
>
> 3) Consistency
>
> Django always uses implicit line continuations instead of backslashes.
> This commits introduces several explicit line continuations with
> backslashes that can't be turned into implicit ones.
>
>
> To be fair, sometimes the new version is more readable :
>
> -with translation.override('ja'):
> -with self.settings(USE_L10N=True):
> -self.humanize_tester([100], ['100'], 'intcomma')
> +with translation.override('ja'), self.settings(USE_L10N=True):
> +self.humanize_tester([100], ['100'], 'intcomma')
>
> But whenever the with statement spills over two lines, which happens in a
> majority of cases, I find it worse than two with statements. It's
> especially bad in the transactions tests — I worked in this area today.
>
> --
> Aymeric.
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> For more options, visit https://groups.google.com/groups/opt_out.
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Revert 165f44aa?

[Aymeric Augustin]

Hello,

I think we went overboard in this commit: 
https://github.com/django/django/commit/165f44aa

Specifically, I consider it's a regression in three areas.


1) Readability

After the patch, indentation no longer matches the logical structure of the 
program. For example:

-with self.settings(USE_L10N=True, USE_THOUSAND_SEPARATOR=False):
-with translation.override('en'):
-self.humanize_tester(test_list, result_list, 'intcomma')
+with self.settings(USE_L10N=True, USE_THOUSAND_SEPARATOR=False), \ 

+translation.override('en'):
+self.humanize_tester(test_list, result_list, 'intcomma')

The new version is longer than the old one (by two characters) and visually 
harder to parse.


2) Readability (bis)

After the patch, distinct operations end up on the same line, obfuscating the 
intent of the code. For example :

-with six.assertRaisesRegex(self, Exception, "Oops"):
-with transaction.atomic():
-Reporter.objects.create(first_name="Haddock")
-raise Exception("Oops, that's his last name")
+with six.assertRaisesRegex(self, Exception, "Oops"), 
transaction.atomic(): 
+Reporter.objects.create(first_name="Haddock")
+raise Exception("Oops, that's his last name")

In the old version, the first line is a test assertion and the second line is 
the code exercised by the test. I find it confusing to put both on the same 
line.


3) Consistency

Django always uses implicit line continuations instead of backslashes. This 
commits introduces several explicit line continuations with backslashes that 
can't be turned into implicit ones.


To be fair, sometimes the new version is more readable :

-with translation.override('ja'):
-with self.settings(USE_L10N=True):
-self.humanize_tester([100], ['100'], 'intcomma')
+with translation.override('ja'), self.settings(USE_L10N=True): 

+self.humanize_tester([100], ['100'], 'intcomma')

But whenever the with statement spills over two lines, which happens in a 
majority of cases, I find it worse than two with statements. It's especially 
bad in the transactions tests — I worked in this area today.

-- 
Aymeric.



-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.