warn/error when using a method as boolean in ifs/whiles
Hey python-ideas, on django-developers, an intriguing idea appeared: https://groups.google.com/d/msg/django-developers/4bntzg1HwwY/HHHjbDnLBQAJ """ It seems to me that the default `method.__bool__` is undesirable in Jinja2 templates. I do not know Jinja2 well enough, but maybe they could benefit from a patch where `if`-statements give a warning/error when the expression is a callable (with the default `FunctionType.__bool__`? This would solve the issue not just for the methods you mention, but more in general. [Or maybe Python itself should have that warning/error?] """ Background: Django implemented form.is_valid as a function. During development, people fall into the trap of believing it's a property or boolean attribute. That's usually not big deal but can take substantial amount of time when writing non trivial code among which reside following innocuous-looking lines: if obj.has_special_property: # will always # be executed What do you think about that Python emitting an warning/error as described above? Cheers, Sven
On 11 October 2016 at 13:41, Sven R. Kunze <srkunze@mail.de> wrote:
Interesting idea. There may be some issues - consider an object that may optionally have a handler method handle_event, and you want to call that method if it exists: handler = getattr(obj, 'handle_event', None) if handler: # prepare arguments handler(args) That could would break (technically, it would produce an incorrect warning) with this change. I do think that the scenario you described is a valid one - and there's no obvious "better name". The stdlib module pathlib uses the same pattern "my_path.is_absolute()", and IIRC I've made the mistake you described (although I don't recall any major trauma, so the problem was probably fixed relatively quickly). I'm not sure: Pros: - Catches an annoying and potentially hard to spot bug Cons: - Would trigger on certain reasonable coding patterns that aren't an error - IMO, "false positives" in warnings are very annoying, particularly in Python where they are runtime rather than compile-time, and so affect the end user (if they aren't fixed in development) Paul
On Tue, Oct 11, 2016 at 7:02 AM, Paul Moore <p.f.moore@gmail.com> wrote:
This isn't a problem if you test for None: "if handler is not None". I was going to concede that there are probably other cases where that doesn't help, but can't think of any. In what case would a callable also evaluate to False, or you have a collection of arbitrary mixed values and you are testing their truthiness? So perhaps always using a None test is the correct answer if we add the warning. That said, I have no illusions that I know all possible use cases... :)
FWIW, I do the same thing from time to time and I usually only catch it when writing unit tests. I'm +1 to a warning as proposed. -eric
On 11 October 2016 at 14:38, Eric Snow <ericsnowcurrently@gmail.com> wrote:
Certainly. But the whole point of the warning is to catch people who do the wrong thing. All I'm saying is that the new warning would cause problems for people who omit a (currently unnecessary) "is None" check in the process of protecting people who forget whether an attribute is a method or a property. Which group do we prefer to help? As I said, I find false positives with warnings particularly annoying, so personally I may find it less acceptable to ask people with working code to tighten it up than others do. It's a trade-off (and although "warns on correct code" is very close to a backward compatibility break[1], I *do* think it's worth considering here - it's just that my personal feelings are mixed). Paul [1] Technically it *is* a break. But I'm willing to give a little wiggle room to the argument that "it's only a warning".
On 11 October 2016 at 23:59, Paul Moore <p.f.moore@gmail.com> wrote:
I don't think there's a technicality here: new warnings on valid code count as a backwards compatibility break, as some folks run their test suites under "-Werror". That means existing correct code wins by default. However, writing a "@predicatemethod" descriptor that folks could use if they wanted to warn about that particular case may make sense. That's already possible for API designers that choose to do it, so I don't see a strong reason to add it to the standard library at this point. (Especially since type inference engines are often going to be able to pick this particular problem up statically) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Tue, Oct 11, 2016 at 02:41:34PM +0200, Sven R. Kunze wrote:
That should be easy enough to do as a custom descriptor. But I would not like to see the default function or method __bool__ raise a warning. Consider processing a sequence of functions/methods, skipping any which are None: for func in callables: if func is not None: func(some_arg) I often written code like that. Now imagine that somebody reasons that since all functions and methods are truthy, and None if falsey, we can write the code as: for func in callables: if func: func(some_arg) That's perfectly reasonable code too, and it should be purely a matter of taste whether you prefer that or the first version. But with this suggestion, we get flooded by spurious warnings. So I think this is something that Django/Jinja2 should implement for its own methods that need it, it should not be a general feature of all Python functions/methods. -- Steve
On 10/11/2016 07:00 AM, Steven D'Aprano wrote:
On Tue, Oct 11, 2016 at 02:41:34PM +0200, Sven R. Kunze wrote:
[...]
Agreed. Python is a /general/-purpose programming language. We should not make changes to help one subset of users when those changes will harm another subset (and being flooded with false positives is harmful) -- particularly when easy customization is already available. -- ~Ethan~
One option would be to decorate those functions and provide an implementation to __bool__ or __nonzero__ which raises an exception. Something like this In [1]: def a(): pass In [2]: def r(): raise RuntimeError('Do not forget to call this') In [3]: a.__bool__ = r In [4]: if a: pass I don't have an environment to test if this is possible. This would allow marking with a decorator functions that might be misleading or that are a common source of issues for new users. -- David Navarro On 11 October 2016 at 17:34, Ethan Furman <ethan@stoneleaf.us> wrote:
-- David Navarro Estruch
On 11 October 2016 at 17:21, David Navarro <davisein@gmail.com> wrote:
It would need to be somewhat more complex (the above doesn't work as it stands, because __bool__ is only recognised as a method on a class, not an attribute of a function). But it's doable, either manually (make a a class with __call__ and __bool__) or semi-automatically (a decorator that creates a class for which __call__ is defined as running the decorated function, and __bool__ raises a warning). Paul
On 11 October 2016 at 13:41, Sven R. Kunze <srkunze@mail.de> wrote:
Interesting idea. There may be some issues - consider an object that may optionally have a handler method handle_event, and you want to call that method if it exists: handler = getattr(obj, 'handle_event', None) if handler: # prepare arguments handler(args) That could would break (technically, it would produce an incorrect warning) with this change. I do think that the scenario you described is a valid one - and there's no obvious "better name". The stdlib module pathlib uses the same pattern "my_path.is_absolute()", and IIRC I've made the mistake you described (although I don't recall any major trauma, so the problem was probably fixed relatively quickly). I'm not sure: Pros: - Catches an annoying and potentially hard to spot bug Cons: - Would trigger on certain reasonable coding patterns that aren't an error - IMO, "false positives" in warnings are very annoying, particularly in Python where they are runtime rather than compile-time, and so affect the end user (if they aren't fixed in development) Paul
On Tue, Oct 11, 2016 at 7:02 AM, Paul Moore <p.f.moore@gmail.com> wrote:
This isn't a problem if you test for None: "if handler is not None". I was going to concede that there are probably other cases where that doesn't help, but can't think of any. In what case would a callable also evaluate to False, or you have a collection of arbitrary mixed values and you are testing their truthiness? So perhaps always using a None test is the correct answer if we add the warning. That said, I have no illusions that I know all possible use cases... :)
FWIW, I do the same thing from time to time and I usually only catch it when writing unit tests. I'm +1 to a warning as proposed. -eric
On 11 October 2016 at 14:38, Eric Snow <ericsnowcurrently@gmail.com> wrote:
Certainly. But the whole point of the warning is to catch people who do the wrong thing. All I'm saying is that the new warning would cause problems for people who omit a (currently unnecessary) "is None" check in the process of protecting people who forget whether an attribute is a method or a property. Which group do we prefer to help? As I said, I find false positives with warnings particularly annoying, so personally I may find it less acceptable to ask people with working code to tighten it up than others do. It's a trade-off (and although "warns on correct code" is very close to a backward compatibility break[1], I *do* think it's worth considering here - it's just that my personal feelings are mixed). Paul [1] Technically it *is* a break. But I'm willing to give a little wiggle room to the argument that "it's only a warning".
On 11 October 2016 at 23:59, Paul Moore <p.f.moore@gmail.com> wrote:
I don't think there's a technicality here: new warnings on valid code count as a backwards compatibility break, as some folks run their test suites under "-Werror". That means existing correct code wins by default. However, writing a "@predicatemethod" descriptor that folks could use if they wanted to warn about that particular case may make sense. That's already possible for API designers that choose to do it, so I don't see a strong reason to add it to the standard library at this point. (Especially since type inference engines are often going to be able to pick this particular problem up statically) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Tue, Oct 11, 2016 at 02:41:34PM +0200, Sven R. Kunze wrote:
That should be easy enough to do as a custom descriptor. But I would not like to see the default function or method __bool__ raise a warning. Consider processing a sequence of functions/methods, skipping any which are None: for func in callables: if func is not None: func(some_arg) I often written code like that. Now imagine that somebody reasons that since all functions and methods are truthy, and None if falsey, we can write the code as: for func in callables: if func: func(some_arg) That's perfectly reasonable code too, and it should be purely a matter of taste whether you prefer that or the first version. But with this suggestion, we get flooded by spurious warnings. So I think this is something that Django/Jinja2 should implement for its own methods that need it, it should not be a general feature of all Python functions/methods. -- Steve
On 10/11/2016 07:00 AM, Steven D'Aprano wrote:
On Tue, Oct 11, 2016 at 02:41:34PM +0200, Sven R. Kunze wrote:
[...]
Agreed. Python is a /general/-purpose programming language. We should not make changes to help one subset of users when those changes will harm another subset (and being flooded with false positives is harmful) -- particularly when easy customization is already available. -- ~Ethan~
One option would be to decorate those functions and provide an implementation to __bool__ or __nonzero__ which raises an exception. Something like this In [1]: def a(): pass In [2]: def r(): raise RuntimeError('Do not forget to call this') In [3]: a.__bool__ = r In [4]: if a: pass I don't have an environment to test if this is possible. This would allow marking with a decorator functions that might be misleading or that are a common source of issues for new users. -- David Navarro On 11 October 2016 at 17:34, Ethan Furman <ethan@stoneleaf.us> wrote:
-- David Navarro Estruch
On 11 October 2016 at 17:21, David Navarro <davisein@gmail.com> wrote:
It would need to be somewhat more complex (the above doesn't work as it stands, because __bool__ is only recognised as a method on a class, not an attribute of a function). But it's doable, either manually (make a a class with __call__ and __bool__) or semi-automatically (a decorator that creates a class for which __call__ is defined as running the decorated function, and __bool__ raises a warning). Paul
participants (7)
-
David Navarro -
Eric Snow -
Ethan Furman -
Nick Coghlan -
Paul Moore -
Steven D'Aprano -
Sven R. Kunze