Issue 29747647: Noissue - Document best practice for formatting multiline statements in Python (Closed)
|
Created:
April 9, 2018, 7:12 p.m. by Vasily Kuznetsov Modified:
April 16, 2018, 9:18 a.m. CC:
tlucas, rosie Visibility:
Public. |
DescriptionNoissue - Document best practice for formatting multiline statements in Python
Repository: https://hg.adblockplus.org/web.adblockplus.org
Base revision: 5f273a5af3d2
Patch Set 1 #Patch Set 2 : Change names from camelCase to underscore_separated #
Total comments: 16
Patch Set 3 : Incorporate Julian's suggestions #
Total comments: 3
MessagesTotal messages: 19
Hi guys,
A long time ago (in the galaxy not so far away) Sebastian and I had a discussion
about Python formatting styles for multiline statements. We've concluded that we
don't like this style:
fooFunction(bar,
baz, qux)
and so we decided to avoid it. However, this decision wasn't documented
anywhere, so when this issue comes up in a review, we need to have the
discussion all over again. So I thought it would be better to add some guidance
to the style guide.
Here I gave it a shot, adding a bit more guidance for comma placement, to make
things more consistent and VCS-friendly. Also it seemed useful to have code
examples, so I added these expanding code sample thingies -- Julian, please see
if you think they are appropriate and if something should be improved.
Thanks in advance for any feedback and suggestions,
Cheers,
Vasily
I agree to the suggested rule (as you pointed out this is what we already enforce in code reviews anyway). Also I like you embedded the examples (requiring a click to show, rather than cluttering the page by default). It could be debated though whether inline scripts are appropriate or whether that code should rather go into an external script. Let's see what Julian says. However, can you please make the variable/method names in the sample snippets PEP-8 compliant (i.e. underscores instead of camelcase)?
On 2018/04/09 20:32:06, Sebastian Noack wrote: > I agree to the suggested rule (as you pointed out this is what we already > enforce in code reviews anyway). Also I like you embedded the examples > (requiring a click to show, rather than cluttering the page by default). > > It could be debated though whether inline scripts are appropriate or whether > that code should rather go into an external script. Let's see what Julian says. > > However, can you please make the variable/method names in the sample snippets > PEP-8 compliant (i.e. underscores instead of camelcase)? OMG, indeed, what was I thinking! :) Fixed.
On 2018/04/09 23:24:13, Vasily Kuznetsov wrote: > On 2018/04/09 20:32:06, Sebastian Noack wrote: > > I agree to the suggested rule (as you pointed out this is what we already > > enforce in code reviews anyway). Also I like you embedded the examples > > (requiring a click to show, rather than cluttering the page by default). > > > > It could be debated though whether inline scripts are appropriate or whether > > that code should rather go into an external script. Let's see what Julian > says. > > > > However, can you please make the variable/method names in the sample snippets > > PEP-8 compliant (i.e. underscores instead of camelcase)? > > OMG, indeed, what was I thinking! :) > Fixed. Do you mind adding me from my email? I have been added to the review from my personal email jsonesen@gmail.com so it makes it harder to keep track of reviews. (My bad for making another account) Also, thanks for doing this, and I agree with Sebastian here about seeing what Julian says, but overall looks pretty good.
On 2018/04/10 00:53:52, jsonesen wrote: > On 2018/04/09 23:24:13, Vasily Kuznetsov wrote: > > On 2018/04/09 20:32:06, Sebastian Noack wrote: > > > I agree to the suggested rule (as you pointed out this is what we already > > > enforce in code reviews anyway). Also I like you embedded the examples > > > (requiring a click to show, rather than cluttering the page by default). > > > > > > It could be debated though whether inline scripts are appropriate or whether > > > that code should rather go into an external script. Let's see what Julian > > says. > > > > > > However, can you please make the variable/method names in the sample > snippets > > > PEP-8 compliant (i.e. underscores instead of camelcase)? > > > > OMG, indeed, what was I thinking! :) > > Fixed. > > Do you mind adding me from my email? I have been added to the review from my > personal email mailto:jsonesen@gmail.com so it makes it harder to keep track of > reviews. (My bad for making another account) Also, thanks for doing this, and I > agree with Sebastian here about seeing what Julian says, but overall looks > pretty good. Sorry, to clarify add from my jon@adblockplus email
(I'll wait for someone else to approve before checking the HTML)
LGTM (for the content)
LGTM + NITs https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:69: <ol> NIT/FWIW: I prefer the one option (the second) instead of two (code style wise). https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} NIT: We like to put "list item" as the context of list item strings https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} NIT: Missing capital? https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:71: (<a onclick="getElementById('python-multiline-a1-example').style.display = 'block';">{{python-multiline-a1-example example}}</a>), or NIT/Suggest: this.nextSibling.classList.toggle('show') where .show{display:block;} (or similar)
Hi Julian, Thanks for the comments. I need a couple of clarifications, see below. Cheers, Vasily https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:69: <ol> On 2018/04/11 13:41:43, juliandoucette wrote: > NIT/FWIW: I prefer the one option (the second) instead of two (code style wise). If I understand you correctly, you would prefer us to mandate the second style as the only style. This would make things more consistent, but there are a couple of arguments against it: - We already use first style a lot and it seems like everyone likes it, - In some cases the second style takes too many lines while the first style is perfectly good and readable. It does make the style guide more complicated, but in this case I think it's worth it. In any case, I'm open to changing this if many Python developers would agree with Julian. https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} On 2018/04/11 13:41:43, juliandoucette wrote: > NIT: Missing capital? I wanted to make preceding text and these two list items to read as one sentence together. If you think that it doesn't really work so well as one sentence, I can also capitalize the first letters and adjust the punctuation. https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} On 2018/04/11 13:41:44, juliandoucette wrote: > NIT: We like to put "list item" as the context of list item strings I'm not sure what you mean here. How would this line change if I put "list item" as the context of the string? https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:71: (<a onclick="getElementById('python-multiline-a1-example').style.display = 'block';">{{python-multiline-a1-example example}}</a>), or On 2018/04/11 13:41:43, juliandoucette wrote: > NIT/Suggest: this.nextSibling.classList.toggle('show') where > .show{display:block;} (or similar) This is nifty! I'd like to change it to this. Should I add .show class to local embedded CSS or to global CSS for the website? Note: it might make more sense to have .hide class instead, since <pre> has {display: block;} by default.
Thanks Vasily! https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:69: <ol> On 2018/04/11 14:51:39, Vasily Kuznetsov wrote: > On 2018/04/11 13:41:43, juliandoucette wrote: > > NIT/FWIW: I prefer the one option (the second) instead of two (code style > wise). > > If I understand you correctly, you would prefer us to mandate the second style > as the only style. This would make things more consistent, but there are a > couple of arguments against it: > > - We already use first style a lot and it seems like everyone likes it, > - In some cases the second style takes too many lines while the first style is > perfectly good and readable. > > It does make the style guide more complicated, but in this case I think it's > worth it. In any case, I'm open to changing this if many Python developers would > agree with Julian. Acknowledged. https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} On 2018/04/11 14:51:39, Vasily Kuznetsov wrote: > On 2018/04/11 13:41:43, juliandoucette wrote: > > NIT: Missing capital? > > I wanted to make preceding text and these two list items to read as one sentence > together. If you think that it doesn't really work so well as one sentence, I > can also capitalize the first letters and adjust the punctuation. Acknowledged. https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} On 2018/04/11 14:51:39, Vasily Kuznetsov wrote: > On 2018/04/11 13:41:44, juliandoucette wrote: > > NIT: We like to put "list item" as the context of list item strings > > I'm not sure what you mean here. How would this line change if I put "list item" > as the context of the string? {{ id[context] content }} https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:71: (<a onclick="getElementById('python-multiline-a1-example').style.display = 'block';">{{python-multiline-a1-example example}}</a>), or On 2018/04/11 14:51:39, Vasily Kuznetsov wrote: > This is nifty! I'd like to change it to this. Should I add .show class to local > embedded CSS or to global CSS for the website? main.css... It's unbelievable that this class doesn't already exist :D. > Note: it might make more sense to have .hide class instead, since <pre> has > {display: block;} by default. Good thinking :)
LGTM content wise, FWIW I prefer to keep the options as they are since this has been the case. However, I am not strongly tied to that idea so if it must be so that is cool with me. Mostly I prefer it since there is so much code which already uses the first option. But I won't be upset if we remove the first option from the style guide and change things going forward.
Hi Julian, I added the context to list item strings and improved the show/hide JavaScript. Thanks for the suggestions. Cheers, Vasily https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} On 2018/04/11 15:06:49, juliandoucette wrote: > On 2018/04/11 14:51:39, Vasily Kuznetsov wrote: > > On 2018/04/11 13:41:44, juliandoucette wrote: > > > NIT: We like to put "list item" as the context of list item strings > > > > I'm not sure what you mean here. How would this line change if I put "list > item" > > as the context of the string? > > {{ id[context] content }} Done. https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:70: <li>{{python-multiline-a1 aligning follow up lines with the opening parentheses and putting the closing parentheses at the end of the last line}} On 2018/04/11 15:06:49, juliandoucette wrote: > On 2018/04/11 14:51:39, Vasily Kuznetsov wrote: > > On 2018/04/11 13:41:44, juliandoucette wrote: > > > NIT: We like to put "list item" as the context of list item strings > > > > I'm not sure what you mean here. How would this line change if I put "list > item" > > as the context of the string? > > {{ id[context] content }} Done. https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:71: (<a onclick="getElementById('python-multiline-a1-example').style.display = 'block';">{{python-multiline-a1-example example}}</a>), or On 2018/04/11 13:41:43, juliandoucette wrote: > NIT/Suggest: this.nextSibling.classList.toggle('show') where > .show{display:block;} (or similar) `this.nextSibling` doesn't seem to work for me and it seems that just referencing the <pre> tag by id will be more robust to possibly future changes, so I left that part as it was. I incorporated the rest of your suggestion though, thanks for the suggestion.
https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29747647/diff/29747664/pages/coding-style.... pages/coding-style.html:71: (<a onclick="getElementById('python-multiline-a1-example').style.display = 'block';">{{python-multiline-a1-example example}}</a>), or On 2018/04/12 12:50:25, Vasily Kuznetsov wrote: > `this.nextSibling` doesn't seem to work for me and it seems that just > referencing the <pre> tag by id will be more robust to possibly future changes, > so I left that part as it was. I incorporated the rest of your suggestion > though, thanks for the suggestion. No worries. I may refactor this separately as show/hiding a section onclick is pretty generic and could be handled globally.
https://codereview.adblockplus.org/29747647/diff/29750578/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29747647/diff/29750578/static/css/main.css... static/css/main.css:808: display: none; Can't we just use use the "hidden" HTML attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden Though it doesn't have any effect on IE 10 (and below), but well having the examples always expanded on ancient versions of IE would be fine IMO.
https://codereview.adblockplus.org/29747647/diff/29750578/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29747647/diff/29750578/static/css/main.css... static/css/main.css:808: display: none; On 2018/04/14 00:41:04, Sebastian Noack wrote: > Can't we just use use the "hidden" HTML attribute: > https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden > > Though it doesn't have any effect on IE 10 (and below), but well having the > examples always expanded on ancient versions of IE would be fine IMO. The `hidden` attribute is not meant to be used in that way AFAICT. See: > The hidden attribute must not be used to hide content that could legitimately be shown in > another presentation. For example, it is incorrect to use hidden to hide panels in a tabbed > dialog, because the tabbed interface is merely a kind of overflow presentation — one could > equally well just show all the form controls in one big page with a scrollbar. It is similarly > incorrect to use this attribute to hide content just from one presentation — if something is > marked hidden, it is hidden from all presentations, including, for instance, screen readers. > > -- https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden > The hidden attribute must not be used to hide content just from one presentation —if > something is marked hidden, it is hidden from all presentations, including, for instance, screen > readers. > > Elements that are not themselves hidden must not hyperlink to elements that are hidden. The > for attributes of label and output elements that are not themselves hidden must similarly not > refer to elements that are hidden. In both cases, such references would cause user confusion. > > -- http://w3c.github.io/html/editing.html#the-hidden-attribute
https://codereview.adblockplus.org/29747647/diff/29750578/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29747647/diff/29750578/static/css/main.css... static/css/main.css:808: display: none; On 2018/04/14 13:17:42, juliandoucette wrote: > On 2018/04/14 00:41:04, Sebastian Noack wrote: > > Can't we just use use the "hidden" HTML attribute: > > https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden > > > > Though it doesn't have any effect on IE 10 (and below), but well having the > > examples always expanded on ancient versions of IE would be fine IMO. > > The `hidden` attribute is not meant to be used in that way AFAICT. > > See: > > > The hidden attribute must not be used to hide content that could legitimately > be shown in > > another presentation. For example, it is incorrect to use hidden to hide > panels in a tabbed > > dialog, because the tabbed interface is merely a kind of overflow presentation > — one could > > equally well just show all the form controls in one big page with a scrollbar. > It is similarly > > incorrect to use this attribute to hide content just from one presentation — > if something is > > marked hidden, it is hidden from all presentations, including, for instance, > screen readers. > > > > -- https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden > > > The hidden attribute must not be used to hide content just from one > presentation —if > > something is marked hidden, it is hidden from all presentations, including, > for instance, screen > > readers. > > > > Elements that are not themselves hidden must not hyperlink to elements that > are hidden. The > > for attributes of label and output elements that are not themselves hidden > must similarly not > > refer to elements that are hidden. In both cases, such references would cause > user confusion. > > > > -- http://w3c.github.io/html/editing.html#the-hidden-attribute Interesting. Then LGTM from my side. (Any reason you didn't give LGTM yet?)
On 2018/04/14 13:24:31, Sebastian Noack wrote: > Interesting. Then LGTM from my side. (Any reason you didn't give LGTM yet?) I gave LGTM in comment #8.
On 2018/04/14 16:45:16, juliandoucette wrote: > On 2018/04/14 13:24:31, Sebastian Noack wrote: > > Interesting. Then LGTM from my side. (Any reason you didn't give LGTM yet?) > > I gave LGTM in comment #8. Well, this was before the latest patch set. But I guess that means, Vasily can push the changes now. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
