py-version should use maxversion and minversion

[clavedeluna]

Current problem

According to pylint

:boolean-datetime (W1502): *Using datetime.time in a boolean context.*
  Using datetime.time in a boolean context can hide subtle bugs when the time
  they represent matches midnight UTC. This behaviour was fixed in Python 3.5.
  See https://bugs.python.org/issue13936 for reference. This message can't be
  emitted when using Python >= 3.5.

Since only py3.7+ versions are supported, I think we can safely delete this checker.

Desired solution

Removal of all boolean-datetime checker code.

Additional context

No response

[Pierre-Sassoulas]

Since only py3.7+ versions are supported,

It's a little more complicated than that : pylint can only be launched with python >= 3.7.2 but it can analyses code for python version older than that using py-version. Keeping this is not a very high maintenance burden so I'd say let's keep it and maybe use the proper py-version to prevent it being raised on interpreter where it's not pertinent.

[clavedeluna]

Makes sense. I was thinking:

    def _check_datetime(self, node: nodes.NodeNG) -> None:
        """Check that a datetime was inferred, if so, emit boolean-datetime warning."""
        if sys.version_info >= (3, 5):
            return
       ....
        ....

Is there a reason to use py-version over sys.version?

[Pierre-Sassoulas]

Is there a reason to use py-version over sys.version?

Yes, if py-version < 3.5 then we need to raise the message still even if the interpreter is 3.10, so py-version is the right tool for that. (By default py-version == sys.version). Do you want to contribute the fix, you already did all the work ? :)

[clavedeluna]

Certainly, I will make a PR!

[clavedeluna]

@Pierre-Sassoulas I'm wondering if this is expected behavior?

for a file named test.py

"""Test boolean-datetime

'py-version' needs to be set to <= '3.5'.
"""
import datetime

if datetime.time(0, 0, 0):  # [boolean-datetime]
    print("datetime.time(0,0,0) is not a bug!")
else:
    print("datetime.time(0,0,0) is a bug!")

if datetime.time(0, 0, 1):  # [boolean-datetime]
    print("datetime.time(0,0,1) is not a bug!")
else:
    print("datetime.time(0,0,1) is a bug!")

pylint test.py --py-version 3.2 gives no output.
For debugging purposes (and to see I wasn't making a mistake) I updated the maxversion for boolean-datetime to {"maxversion": (3, 10)}, instead of 3, 5 and I actually do get an output!

************* Module test
test.py:7:3: W1502: Using datetime.time in a boolean context. (boolean-datetime)
test.py:12:3: W1502: Using datetime.time in a boolean context. (boolean-datetime)

So I'm wondering what's going on? Is the code that handles maxversion not working right?

I don't want to create a PR until I understand current behavior.

[Pierre-Sassoulas]

I updated the maxversion for boolean-datetime to {"maxversion": (3, 10)},

Are we talking about a functional test configuration, the property of the checker, or something else ?

[clavedeluna]

I updated the maxversion for boolean-datetime to {"maxversion": (3, 10)},

Are we talking about a functional test configuration, the property of the checker, or something else ?

In this statement what I mean is I literally went to this line and locally updated it from 3,5 to 3, 10. This was the only was able to see boolean-datetime in the output, despite having tried passing --py-version 3.2 before changing the code. I thought py-version is meant for requesting pylint to check for that passed py version, regardless of the sys python version? If so, passing --py-version 3.2 (even tho I'm using python 3.8) I expected boolean-datetime code to fire.

[DanielNoord]

That maxversion should be removed and a check for py-version should be used. Nice catch!

[Pierre-Sassoulas]

Thank you for bringing up the issue. I think you found an issue with pylint's core. It sound like py-version was not well thought to work well with max-version or other messages option. py-version is new and max-version is not used a lot which is probably why it's been bugged for 3/4 minor without us realizing. I'm thinking that py-version should replace max-version or take it into account (to avoid a deprecation of max-version). It's probably better to do it in a new issue to discuss the design with multiple maintainers first.

[DanielNoord]

I don't think a separate issue is needed. The maxversion is a good way to stop emitting messages for versions that shouldn't emit. However, with py-version we can control this much more fine-grained and across interpreters. That's a good thing! Let's move towards it!

[Pierre-Sassoulas]

Sure, but it's included in the API of the checkers, some plugins are probably using this value. I agree that py-version is better.

[DanielNoord]

Sure, but it's included in the API of the checkers, some plugins are probably using this value. I agree that py-version is better.

Oh yeah I didn't mean removing it as a functionality!

[Pierre-Sassoulas]

What I have in mind right now is taking max-version correctly into account for py-version and linking the two (in the way @clavedeluna expected them to be linked). What do you think ?

[DanielNoord]

I'm not sure I understand what you mean with linking.

[Pierre-Sassoulas]

If max-version == 3.5 then the message would not be raised unless py-version is <= 3.5 (right now it seems it's 'unless sys.version <= 3.5' or worse the opposite of that if I understand what @clavedeluna said).

[DanielNoord]

Ah yeah, that should work. Although it might be better to just refactor the few messages that actually use this? I'm not sure how extensible the max version code is.

[Pierre-Sassoulas]

What would we add in message definition to specify their maximum and minimum py-version directly (https://github.com/PyCQA/pylint/blob/main/pylint/checkers/stdlib.py#L362) ? This setting seem like a clean way to specify the py-version without hard coding it in the check with a if config.py_version .... What I meant by linking the two was taking into account the max-version / min-version setting based on the value of py-version instead of the sys.version.

[clavedeluna]

Seems to me you're all saying we should move toward maxversion being a subset of py-version, with minversion being another subset. Max/min version would create an upper/lower bound on py-version.

Some test cases to set us stragit:

maxversion=3.5 for boolean-datetime, --py-version 3.5 passed: message in output
maxversion=3.5 for boolean-datetime, --py-version 3.6 passed: message in NOT output
maxversion=3.5 for boolean-datetime, --py-version 3.2 passed: message in output

minversion=3.6 for used-prior-global-declaration, --py-version 3.5 passed: message NOT in output
minversion=3.6 for used-prior-global-declaration, --py-version 3.7 passed: message in output
...

do you agree?

[Pierre-Sassoulas]

The use cases are ✔️