Issue 29569630: Noissue - Added flake8-docstrings (sitescripts) (Closed)
|
Created:
Oct. 9, 2017, 1:32 a.m. by Sebastian Noack Modified:
Oct. 11, 2017, 5:17 p.m. Reviewers:
Vasily Kuznetsov CC:
tlucas Visibility:
Public. |
DescriptionNoissue - Added flake8-docstrings (sitescripts)
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 6
https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:135: command = ['hg', 'clone', '-q', self.config.repository, '-u', For some reason flake8 didn't detect this redundant space before installing flake8-docstrings, even though it isn't a D* error. https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini#newcode15 tox.ini:15: ignore = D1 D1 is for missing docstrings. I think enforcing docstrings on every class/function/module goes to far. Let's just make sure that existing docstrings comply to PEP-257.
LGTM https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:135: command = ['hg', 'clone', '-q', self.config.repository, '-u', On 2017/10/09 01:56:17, Sebastian Noack wrote: > For some reason flake8 didn't detect this redundant space before installing > flake8-docstrings, even though it isn't a D* error. This is really strange... how are we supposed to trust it after this :/ https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini#newcode15 tox.ini:15: ignore = D1 On 2017/10/09 01:56:17, Sebastian Noack wrote: > D1 is for missing docstrings. I think enforcing docstrings on every > class/function/module goes to far. Let's just make sure that existing docstrings > comply to PEP-257. We should decide what the policy should be for new code. I would say that everything that's exported from a module (content of __all__, or everything that doesn't start with an underscore if there's no __all__) definitely should have a docstring, and maybe all toplevel functions as well as all non-private methods also should have docstrings. In any case, for codebases like sitescripts blanket ignore of D1 makes sense at this point.
https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini#newcode15 tox.ini:15: ignore = D1 On 2017/10/09 20:39:47, Vasily Kuznetsov wrote: > On 2017/10/09 01:56:17, Sebastian Noack wrote: > > D1 is for missing docstrings. I think enforcing docstrings on every > > class/function/module goes to far. Let's just make sure that existing > docstrings > > comply to PEP-257. > > We should decide what the policy should be for new code. I would say that > everything that's exported from a module (content of __all__, or everything that > doesn't start with an underscore if there's no __all__) definitely should have a > docstring, and maybe all toplevel functions as well as all non-private methods > also should have docstrings. In any case, for codebases like sitescripts blanket > ignore of D1 makes sense at this point. For library projects, I completely agree. However, most of our modules and their functions/classes are not meant to be imported by other projects, and it this case I usually advise: Add a docstring if you have anything to add that helps to understand the code, don't add a docstring if it doesn't add anything that isn't obvious from a quick look at the code.
Message was sent while issue was closed.
On 2017/10/10 04:38:32, Sebastian Noack wrote: > https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini > File tox.ini (right): > > https://codereview.adblockplus.org/29569630/diff/29569631/tox.ini#newcode15 > tox.ini:15: ignore = D1 > On 2017/10/09 20:39:47, Vasily Kuznetsov wrote: > > On 2017/10/09 01:56:17, Sebastian Noack wrote: > > > D1 is for missing docstrings. I think enforcing docstrings on every > > > class/function/module goes to far. Let's just make sure that existing > > docstrings > > > comply to PEP-257. > > > > We should decide what the policy should be for new code. I would say that > > everything that's exported from a module (content of __all__, or everything > that > > doesn't start with an underscore if there's no __all__) definitely should have > a > > docstring, and maybe all toplevel functions as well as all non-private methods > > also should have docstrings. In any case, for codebases like sitescripts > blanket > > ignore of D1 makes sense at this point. > > For library projects, I completely agree. However, most of our modules and their > functions/classes are not meant to be imported by other projects, and it this > case I usually advise: Add a docstring if you have anything to add that helps to > understand the code, don't add a docstring if it doesn't add anything that isn't > obvious from a quick look at the code. In general this is a sensible approach, but one needs to be careful when judging what is and what is not obvious "from a quick look at the code". From my experience with Buildtools, CMS and Sitescripts, I can say that many things took some time to figure out. In general, things that seem obvious to a programmer familiar with the code and having the relevant background knowledge are often not "obvious from a quick look at the code" when you see it for the first time. Some of that is essential complexity and things that just don't make sense without background knowledge, some part cannot be solved well with documentation and has more to do with the structure of the code, but documentation also has a role to play. Even though maintainer-oriented documentation of projects that are not libraries doesn't always fit very well into docstrings, there are quite a few cases when it does. I don't think there's a way to enforce this with tooling (so I agree on ignoring D1 for non-libraries), but maybe we can address this in our coding style guide.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:135: command = ['hg', 'clone', '-q', self.config.repository, '-u', On 2017/10/09 20:39:47, Vasily Kuznetsov wrote: > On 2017/10/09 01:56:17, Sebastian Noack wrote: > > For some reason flake8 didn't detect this redundant space before installing > > flake8-docstrings, even though it isn't a D* error. > > This is really strange... how are we supposed to trust it after this :/ I figured out what is going on here. This wasn't related to installing flake8-docstrings. Everything works as intended. By default flake8 (and pycodingstyles) ignore E24 (along some other errors, i.e. E121, E123, E126, E226, E704, W503 and W504). That is because PEP-8 doesn't explicitly mention/discourage these practices. However, I went through the list, and every single of these errors is something that you and I would complain about, if noticed in a code reviews. So it is good that these are no longer ignored. The reason why they are no longer ignored is that we added `ignore = D1` to the flake8 section in tox.ini, overriding any pre-defined ignores. If we wouldn't have at least one global ignore in each project (i.e. D1), now, I would suggest to add `ignore =` to each project that doesn't have anything to ignore, in order to get the above errors.
Message was sent while issue was closed.
On 2017/10/10 22:01:48, Sebastian Noack wrote: > https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... > File sitescripts/extensions/bin/createNightlies.py (right): > > https://codereview.adblockplus.org/29569630/diff/29569631/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:135: command = ['hg', 'clone', > '-q', self.config.repository, '-u', > On 2017/10/09 20:39:47, Vasily Kuznetsov wrote: > > On 2017/10/09 01:56:17, Sebastian Noack wrote: > > > For some reason flake8 didn't detect this redundant space before installing > > > flake8-docstrings, even though it isn't a D* error. > > > > This is really strange... how are we supposed to trust it after this :/ > > I figured out what is going on here. This wasn't related to installing > flake8-docstrings. Everything works as intended. By default flake8 (and > pycodingstyles) ignore E24 (along some other errors, i.e. E121, E123, E126, > E226, E704, W503 and W504). That is because PEP-8 doesn't explicitly > mention/discourage these practices. However, I went through the list, and every > single of these errors is something that you and I would complain about, if > noticed in a code reviews. So it is good that these are no longer ignored. > > The reason why they are no longer ignored is that we added `ignore = D1` to the > flake8 section in tox.ini, overriding any pre-defined ignores. If we wouldn't > have at least one global ignore in each project (i.e. D1), now, I would suggest > to add `ignore =` to each project that doesn't have anything to ignore, in order > to get the above errors. Awesome! Then we can still trust flake8 :D 
|
