Issue 29340645: Noissue - Extend Python style guide (Closed)
|
Created:
April 20, 2016, 10:51 a.m. by Sebastian Noack Modified:
May 11, 2016, 5:34 p.m. Reviewers:
Vasily Kuznetsov CC:
Felix Dahlke, kzar Visibility:
Public. |
DescriptionNoissue - Extend Python style guide
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments and added more points #
Total comments: 7
Patch Set 3 : Addressed comments #MessagesTotal messages: 6
https://codereview.adblockplus.org/29340645/diff/29340646/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29340645/diff/29340646/pages/coding-style.... pages/coding-style.html:50: <li>{{python-regexp Use <a href="https://docs.python.org/2/library/re.html#re.search"><code><fix>re.search()</fix></code></a> instead <a href="https://docs.python.org/2/library/re.html#re.match"><code><fix>re.match()</fix></code></a> to avoid confusion about implied begin of string.}}</li> I think "beginning of the string" would be better than "begin of string". Also, perhaps we should add "but not the ending" (so the final result would be "to avoid confusion about implied beginning of the string but not the ending") -- at least for me the confusion is actually about the inconsistency and not doing what "match" implies (string as a whole matches the regexp). Not sure if we should also reference this part of the python doc: https://docs.python.org/2/library/re.html#search-vs-match -- you do already give two links so perhaps that's enough.
I also added some more points we meanwhile agreed on. https://codereview.adblockplus.org/29340645/diff/29340646/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29340645/diff/29340646/pages/coding-style.... pages/coding-style.html:50: <li>{{python-regexp Use <a href="https://docs.python.org/2/library/re.html#re.search"><code><fix>re.search()</fix></code></a> instead <a href="https://docs.python.org/2/library/re.html#re.match"><code><fix>re.match()</fix></code></a> to avoid confusion about implied begin of string.}}</li> On 2016/04/20 12:11:18, Vasily Kuznetsov wrote: > I think "beginning of the string" would be better than "begin of string". > > Also, perhaps we should add "but not the ending" (so the final result would be > "to avoid confusion about implied beginning of the string but not the ending") > -- at least for me the confusion is actually about the inconsistency and not > doing what "match" implies (string as a whole matches the regexp). Yeah, I agree. I was just tried to keep in short, but fair enough. > Not sure if we should also reference this part of the python doc: > https://docs.python.org/2/library/re.html#search-vs-match -- you do already give > two links so perhaps that's enough. I added that link and removed the link to re.match. No point linking to docs of an API we advise against anyway. https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:46: <li>{{python-general Follow <a href="https://www.python.org/dev/peps/pep-0008/">PEP-8</a> and the recommendations in the offical Python documentation.}}</li> I removed the reference to Mozilla's Python practices, since there is nothing left that isn't overridden/duplicated here or have never done by us.
There are some small edits but otherwise it all looks good! https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:47: <li>{{python-strings Write string literals so that they match the behaviour of <a href="https://docs.python.org/2/library/functions.html#func-repr"><code><fix>repr()</fix></code></a> in Python 2, or <a href="https://docs.python.org/3/library/functions.html#ascii"><code><fix>ascii()</fix></code></a> in Python 3, i.e. use single quotes except to avoid escaping of embedded quotes and escape special and non-ascii characters. For docstrings however follow <a href="https://www.python.org/dev/peps/pep-0257/">PEP-257</a>.}}</li> Perhaps add commas around "however" in the last sentence or even remove "however" altogether. Maybe also the docstrings sentence could be a separate bullet? https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:52: <li>{{python-map-filter Use list comprehensions or generator expressions instead calling <code><fix>map()</fix></code> or <code><fix>filter()</fix></code> with a lambda function.}}</li> "instead of"? :) https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:53: <li>{{python-regexp Use <a href="https://docs.python.org/2/library/re.html#re.search"><code><fix>re.search()</fix></code></a> instead <code><fix>re.match()</fix></code> to avoid <a href="https://docs.python.org/2/library/re.html#search-vs-match">confusion</a> about implied beginning of the string but not the ending.}}</li> And here too, "instead of".
https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:47: <li>{{python-strings Write string literals so that they match the behaviour of <a href="https://docs.python.org/2/library/functions.html#func-repr"><code><fix>repr()</fix></code></a> in Python 2, or <a href="https://docs.python.org/3/library/functions.html#ascii"><code><fix>ascii()</fix></code></a> in Python 3, i.e. use single quotes except to avoid escaping of embedded quotes and escape special and non-ascii characters. For docstrings however follow <a href="https://www.python.org/dev/peps/pep-0257/">PEP-257</a>.}}</li> On 2016/05/11 16:55:42, Vasily Kuznetsov wrote: > Perhaps add commas around "however" in the last sentence or even remove > "however" altogether. Maybe also the docstrings sentence could be a separate > bullet? Well, it's an exception to the rule in the first sentence. So I think it belongs here and that the "however" is essential. https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:52: <li>{{python-map-filter Use list comprehensions or generator expressions instead calling <code><fix>map()</fix></code> or <code><fix>filter()</fix></code> with a lambda function.}}</li> On 2016/05/11 16:55:42, Vasily Kuznetsov wrote: > "instead of"? :) Done. https://codereview.adblockplus.org/29340645/diff/29341241/pages/coding-style.... pages/coding-style.html:53: <li>{{python-regexp Use <a href="https://docs.python.org/2/library/re.html#re.search"><code><fix>re.search()</fix></code></a> instead <code><fix>re.match()</fix></code> to avoid <a href="https://docs.python.org/2/library/re.html#search-vs-match">confusion</a> about implied beginning of the string but not the ending.}}</li> On 2016/05/11 16:55:42, Vasily Kuznetsov wrote: > And here too, "instead of". Done.
LGTM 
|
