Two attribute usage checkers I'd like to see
These features may already be possible with some tools. If so, pointers appreciated. When a class evolves over a long period of time, it's quite possible for methods or data attributes to fall into disuse. While I realize that attributes which don't start with an underscore are implicitly part of the class's API, most attributes are used internally. I'd like to be able to have my checker(s) warn me if I define an attribute but don't use it within the class: class Foo: def __init__(self): self.x = 0 def y(self): pass .... If I never refer to self.x or self.y within the class's methods it would be nice to be alerted. It might catch misspellings (think_long_amd_meaningful_attribute_names) or catch what is effectively dead code. Obviously, this checker should be something you can turn on and off selectively. There are plenty of cases where attributes aren't used within the class implementation but are used by its clients. This is not a hard-and-fast rule. Here's the other problem I'd like to catch: class X(object): ... def get_foo(self): return self._foo foo = property(get_foo) The problem here is that I have violated this Zen of Python dictum: There should be one-- and preferably only one --obvious way to do it. I will admit that I sometimes go back and forth on property objects. On the one hand, as a Python programmer, I like them. I didn't always feel that way though, and some of the people I have worked with over the years have been more C++-centric, and tended to spell their setters and getters without leading underscores. Skip
On 7 June 2013 06:49, Skip Montanaro <skip@pobox.com> wrote:
If I never refer to self.x or self.y within the class's methods it would be nice to be alerted.
+1, and that seems feasible to add to pylint.
Here's the other problem I'd like to catch: class X(object): ... def get_foo(self): return self._foo foo = property(get_foo) The problem here is that I have violated this Zen of Python dictum: There should be one-- and preferably only one --obvious way to do it.
I will admit that I sometimes go back and forth on property objects. But there is no obvious one way to do this in Python<http://stackoverflow.com/q/6618002/243712>. Depending on your project's standards you could say any of - don't use trivial getters/setters or properties, just have a public field (consenting adults) - don't use properties, use an explicit method call (because calls that may take time or have side effects should not look like a simple attribute read) - don't assign property objects to an attribute, use @property - the property getter implementation shouldn't be public - a property getter implementation that only reads a variable should be an inlined lambda - ... should be an operator.attrgetter (I realize your example of just getting a field value may be unrealistically simplified, but even if the getter was doing a lot of work several of those options are available.) -- Martin
Here's the other problem I'd like to catch: class X(object): ... def get_foo(self): return self._foo foo = property(get_foo) The problem here is that I have violated this Zen of Python dictum: There should be one-- and preferably only one --obvious way to do it.
I will admit that I sometimes go back and forth on property objects.
But there is no obvious one way to do this in Python.
Correct, but I would just like to be alerted that perhaps I should consider narrowing my API. Some of the code base I work on was around before Python 2.2 was available. Over time, people have started to add property-based setters and getters. In many circumstances, we should probably pick one way to do it, in this case self.foo vs self.get_foo(). I'm just looking for the message to remind me to consider the API duplication. I'm not suggesting pylint or other tools should recommend one or the other. Skip
On 7 June 2013 09:32, Skip Montanaro <skip@pobox.com> wrote:
Correct, but I would just like to be alerted that perhaps I should consider narrowing my API. Some of the code base I work on was around before Python 2.2 was available. Over time, people have started to add property-based setters and getters. In many circumstances, we should probably pick one way to do it, in this case self.foo vs self.get_foo(). I'm just looking for the message to remind me to consider the API duplication. I'm not suggesting pylint or other tools should recommend one or the other.
That makes sense. You could do something similar when people are migrating methods but the old one's hanging around. -- Martin
On 07 juin 09:18, Martin Pool wrote:
On 7 June 2013 06:49, Skip Montanaro <skip@pobox.com> wrote:
If I never refer to self.x or self.y within the class's methods it would be nice to be alerted.
+1, and that seems feasible to add to pylint.
Hey guys, today is your last chance to contribute this to the pylint 10th anniversary sprint ;) And to get it in forthcoming 1.0... Monday and tuesday reports: https://www.logilab.org/blogentry/146924 https://www.logilab.org/blogentry/147339 -- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
I don't think I can make an actual patch, but can I suggest a very simple change? This message: Invalid name "f" for type variable (should match [a-z_][a-z0-9_]{2,30}$) would be simpler and clearer without the word "type". I always read it first as "f is not a valid name for a type". Can we shorten it to: Invalid name "f" for variable (should match [a-z_][a-z0-9_]{2,30}$) --Ned. On 6/19/2013 3:37 AM, Sylvain Thénault wrote:
On 07 juin 09:18, Martin Pool wrote:
On 7 June 2013 06:49, Skip Montanaro <skip@pobox.com> wrote:
If I never refer to self.x or self.y within the class's methods it would be nice to be alerted.
+1, and that seems feasible to add to pylint. Hey guys, today is your last chance to contribute this to the pylint 10th anniversary sprint ;) And to get it in forthcoming 1.0...
Monday and tuesday reports: https://www.logilab.org/blogentry/146924 https://www.logilab.org/blogentry/147339
On 19 juin 06:59, Ned Batchelder wrote:
I don't think I can make an actual patch, but can I suggest a very simple change? This message:
Invalid name "f" for type variable (should match [a-z_][a-z0-9_]{2,30}$)
would be simpler and clearer without the word "type". I always read it first as "f is not a valid name for a type". Can we shorten it to:
Invalid name "f" for variable (should match [a-z_][a-z0-9_]{2,30}$)
Good suggestion, I've just changed so you'll get in case of the above example: Invalid variable name "f" So it's even shorter (we decided to drop the regexp pattern as well since it's not easily readable and may be a long string). -- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
On Wed, Jun 19, 2013 at 2:37 AM, Sylvain Thénault <sylvain.thenault@logilab.fr> wrote:
On 07 juin 09:18, Martin Pool wrote:
On 7 June 2013 06:49, Skip Montanaro <skip@pobox.com> wrote:
If I never refer to self.x or self.y within the class's methods it would be nice to be alerted.
+1, and that seems feasible to add to pylint.
Hey guys, today is your last chance to contribute this to the pylint 10th anniversary sprint ;) And to get it in forthcoming 1.0...
Monday and tuesday reports: https://www.logilab.org/blogentry/146924 https://www.logilab.org/blogentry/147339
-- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
I don't have a bunch of spare time, but can you point me in the general direction of where such a check would be made? I believe pylint already has a "set but not used" message for local variables. This check could, I think, be patterned after that code. Sorry, I wasn't aware you were sprinting. Skip
On 19 juin 10:41, Skip Montanaro wrote:
On Wed, Jun 19, 2013 at 2:37 AM, Sylvain Thénault <sylvain.thenault@logilab.fr> wrote:
On 07 juin 09:18, Martin Pool wrote:
On 7 June 2013 06:49, Skip Montanaro <skip@pobox.com> wrote:
If I never refer to self.x or self.y within the class's methods it would be nice to be alerted.
+1, and that seems feasible to add to pylint.
Hey guys, today is your last chance to contribute this to the pylint 10th anniversary sprint ;) And to get it in forthcoming 1.0...
Monday and tuesday reports: https://www.logilab.org/blogentry/146924 https://www.logilab.org/blogentry/147339
-- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
I don't have a bunch of spare time, but can you point me in the general direction of where such a check would be made? I believe pylint already has a "set but not used" message for local variables. This check could, I think, be patterned after that code.
There is an unused-variable message in the variables checker (pylint/checkers/variables.py). This is probably worth reading indeed, though in this particular case in should rather go in the classes checker (pylint/checkers/classes.py).
Sorry, I wasn't aware you were sprinting.
no problem. I will do a whole sprint report later today. -- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
There is an unused-variable message in the variables checker (pylint/checkers/variables.py). This is probably worth reading indeed, though in this particular case in should rather go in the classes checker (pylint/checkers/classes.py).
Thanks for the pointer. I think I have a notion of how to go about this, however, I lacked up-to-date source. I cloned logilab-common, pylint and astroid. Installed in ~/.local, then tried pylint with PYTHONPATH set, but no command line args. No luck: % PYTHONPATH=/home/skipm/.local/lib/python2.7/site-packages pylint Traceback (most recent call last): File "/home/skipm/.local/bin/pylint", line 6, in <module> from pkg_resources import load_entry_point File "/home/skipm/.local/lib/python2.7/site-packages/pkg_resources.py", line 2805, in <module> working_set.require(__requires__) File "/home/skipm/.local/lib/python2.7/site-packages/pkg_resources.py", line 696, in require needed = self.resolve(parse_requirements(requirements)) File "/home/skipm/.local/lib/python2.7/site-packages/pkg_resources.py", line 594, in resolve raise DistributionNotFound(req) pkg_resources.DistributionNotFound: logilab-astroid>=0.24.3 udesktop267% PYTHONPATH=/home/skipm/.local/lib/python2.7/site-packages python Python 2.7.2 (default, Oct 17 2012, 03:11:33) [GCC 4.4.6 [TWW]] on sunos5 Type "help", "copyright", "credits" or "license" for more information.
import astroid astroid.__file__ '/home/skipm/.local/lib/python2.7/site-packages/astroid-0.24.3-py2.7.egg/astroid/__init__.pyc'
I know this isn't a distutils mailing list and that you all are sprinting, so you're busy thinking about other stuff, however... Any idea how I can get past this basic hurdle so I can actually do some work with pylint? (Is there some non-pkg_resources way to install these things?) Thx, Skip
I cloned logilab-common, pylint and astroid. Installed in ~/.local, then tried pylint with PYTHONPATH set, but no command line args. No luck:
...
pkg_resources.DistributionNotFound: logilab-astroid>=0.24.3
I believe I have this figured out. Pylint appears to be requiring a package named logilab-astroid, while the astroid package is installed as simply "astroid". I manually edited the installed EGG-INFO/requires.txt to refer to "astroid". I have no idea how this is generated, but I at least have a workaround until the problem can be fixed or someone can explain what I was doing wrong. Skip
On 20 juin 14:55, Skip Montanaro wrote:
I cloned logilab-common, pylint and astroid. Installed in ~/.local, then tried pylint with PYTHONPATH set, but no command line args. No luck:
...
pkg_resources.DistributionNotFound: logilab-astroid>=0.24.3
I believe I have this figured out. Pylint appears to be requiring a package named logilab-astroid, while the astroid package is installed as simply "astroid". I manually edited the installed EGG-INFO/requires.txt to refer to "astroid". I have no idea how this is generated, but I at least have a workaround until the problem can be fixed or someone can explain what I was doing wrong.
This is a bug in pylint's packaging, I've just pushed a fix. -- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
On 19 June 2013 17:37, Sylvain Thénault <sylvain.thenault@logilab.fr> wrote:
Hey guys, today is your last chance to contribute this to the pylint 10th anniversary sprint ;) And to get it in forthcoming 1.0...
Monday and tuesday reports: https://www.logilab.org/blogentry/146924 https://www.logilab.org/blogentry/147339
I think I missed it, but thanks for the updates. I'm especially pleased to hear Torsten pushing out some checkers from Google's pylint. -- Martin
participants (4)
-
Martin Pool -
Ned Batchelder -
Skip Montanaro -
Sylvain Thénault