Require Python 3.6 for consider f-string check

[cdce8p]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Don't emit consider-using-f-string if py-version is set to < 3.6.

Closes #5019

/CC: @DanielNoord

[DanielNoord]

Although @Pierre-Sassoulas did remove all tests with py35 recently, this looks good.

However, I think there might be a discussion to be had about making checkers backwards compatible. This checker was designed in 2.10 which only supports python > 3.6. Adding checks for unsupported Python versions does not seem logical and is not what (new) contributors will expect.
Personally I believe that we should make py-version emit a warning if it is set to an unsupported Python version. What if a user sets it to 3.4 and gets errors because stuff is broken? This new option seems to create expectations about functionality that is not included.

[coveralls]

Pull Request Test Coverage Report for Build 1246284932

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• 4 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.002%) to 93.059%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/reporters/base_reporter.py	4	88.24%

Totals
Change from base Build 1246220533:	0.002%
Covered Lines:	13327
Relevant Lines:	14321

---

💛 - Coveralls

[cdce8p]

Although @Pierre-Sassoulas did remove all tests with py35 recently, this looks good.

Please see the discussion in #5019. The added test is explicitly to be executed with Python 3.6+

However, I think there might be a discussion to be had about making checkers backwards compatible. This checker was designed in 2.10 which only supports python > 3.6. Adding checks for unsupported Python versions does not seem logical and is not what (new) contributors will expect.

I don't agree. Although we don't support running pylint with an old version that doesn't mean users will also abandon support in their projects. If they run pylint with a supported version, it shouldn't really matter. I'm not proposing adding any new checks for old versions, removing them was the right call IMO.

To underscore my point, consider the following: We add a check to suggest the use of the new match / case (added in 3.10). It would be useless to almost everyone as they have to support older versions. Does that mean though we can't add the check at all?

This new option seems to create expectations about functionality that is not included.

It might only seem that way at first. pylint doesn't have that many checks that suggest the use of new features. We only started adding them recently. consider-using-f-string is one, I added consider-using-assignment-expr and a while back the Typing extension checks.
We might be missing a few, but support for them can be added later (once we're aware of the issue).

[scop]

I believe the check doesn't actually require Python 3.6, but that py-version is set to >= that. In that sense, this is misleading.

Same problem exists in R6103 docs (and ChangeLog and whatsnew entry for it). Fixed also in a few related places below.

Suggested change

	"Requires Python 3.6",
	"Requires py-version >= 3.6",

[cdce8p]

I see what you mean and how it can be misleading. However, I don't think using py-version would be better. Technically f-strings do require Python 3.6. Most users shouldn't need to set py-version as long as they run pylint with the oldest version that their project supports.

Another argument, if a project still supports Python 3.5, it's also correct to state that 3.6 is required.

[scop]

Most users shouldn't need to set py-version as long as they run pylint with the oldest version that their project supports.

Yup. But is there any documentation that would hint or explicitly say this would be the recommended thing to do? How could users tell? Unless one is familiar with the tool internals, going with the latest Python would be arguably an equally good or even better choice on surface.

For the f-strings case the Python version vs py-version distinction is coincidentally not that relevant if pylint won't run with earlier than 3.6. But saying "Requires Python 3.8" for a feature that can be checked with pylint running with Python 3.6 perfectly well it wouldn't be so. I haven't actually checked if such a case exists currently (i.e. if one can get consider-using-assignment-expr checked and messages when running with 3.6), but talking based on a hunch that it could. For that case I don't think anything besides referring to "py-version >= 3.8" would be the right thing to do, and for the same reasons, ignoring the noted coincidence, I still think documenting in terms of py-version would be the better choice here as well.

[scop]

(Anyway, happy to see this went in, thanks a bunch for that! Doc matters such as the one being discussed here, while important, are a secondary concern.)

[cdce8p]

Yup. But is there any documentation that would hint or explicitly say this would be the recommended thing to do?

Not that I know of. That is certainly an area that needs improvement. See #5038

How could users tell? Unless one is familiar with the tool internals, going with the latest Python would be arguably an equally good or even better choice on surface.

I generalized py-version exactly for that use case. For pylint and astroid we have a pylint pre-commit hook that uses the local pylint install. I'm using Python 3.10 at the moment, so every time pre-commit would fail for things I couldn't fix as pylint still supports Python 3.6.

But saying "Requires Python 3.8" for a feature that can be checked with pylint running with Python 3.6 perfectly well it wouldn't be so. I haven't actually checked if such a case exists currently (i.e. if one can get consider-using-assignment-expr checked and messages when running with 3.6), but talking based on a hunch that it could. For that case I don't think anything besides referring to "py-version >= 3.8" would be the right thing to do, and for the same reasons, ignoring the noted coincidence, I still think documenting in terms of py-version would be the better choice here as well.

consider-using-assignment-expr could indeed be checked with pylint on 3.6. However, I think of it differently. If a user uses Python 3.6 for pylint, he/she most likely also still supports it for their project. (Otherwise they wouldn't be using 3.6 in the first place.) Thus by saying Requires Python 3.8, even a beginner should now that he/she needs to update their Python install first.

In contrast, using py-version >= 3.8 might lead to a situation where they think they can just set and start using it without updating Python. Although (hopefully) not that common, I imaging that would be quite frustrating.

[scop]

Otherwise they wouldn't be using 3.6 in the first place.

That is a fair point. I don't quite agree with the rest of the argumentation though, I think it trades potential beginner friendliness for inaccuracy, and for project like pylint, to me the latter is vastly more important. But I'm happy to leave it for the pylint maintainers to decide, just wanted to raise the point and thought I'd say what I'd do if it was my call.

[Pierre-Sassoulas]

Maybe we can be very specific ? It would probably prevent multiple stackoverflow question or pylint's issues. It's not like we have a limited number of characters available 😄 . "Requires a python interpreter >= 3.6 to work and the pylint option 'py-version' set to >= 3.6 to be enabled" ?

[scop]

Just a note, the former "to work" part in that wording would be ambiguous -- it's not clear from that alone whether it means the check would work, or f-strings would work. "f-strings require Python >= 3.6, and this check is enabled only if the pylint option py-version is set to >= 3.6" would be more accurate if you want to go that route.

[Pierre-Sassoulas]

LGTM :)

[Pierre-Sassoulas]

I was afraid of the potential for unexpected compatibility work to do but maybe you're right. The tests matrix just got enormous though. Do we need to check functional tests with each py-version ? It seems like we should but this would add maybe 5mn per pipeline.

[DanielNoord]

It might only seem that way at first. pylint doesn't have that many checks that suggest the use of new features. We only started adding them recently. consider-using-f-string is one, I added consider-using-assignment-expr and a while back the Typing extension checks.
We might be missing a few, but support for them can be added later (once we're aware of the issue).

I misunderstood the purpose of this option. It allows users which are on >= 3.6 to check if their code would be good on < 3.6 (or any other version), right? Then I do indeed think this is fine. Sorry for the confusion!

I was afraid of the potential for unexpected compatibility work to do but maybe you're right. The tests matrix just got enormous though. Do we need to check functional tests with each py-version ? It seems like we should but this would add maybe 5mn per pipeline.

Perhaps we could make one functional test for py-version <= 3.5? Instead of splitting it over multiple additional files, have one file test that all warnings which shouldn't be emitted on 3.5 or lower are not emitted? This goes against the idea of making functional tests for each individual checker, but might be a cleaner solution.

[cdce8p]

I misunderstood the purpose of this option. It allows users which are on >= 3.6 to check if their code would be good on < 3.6 (or any other version), right? Then I do indeed think this is fine. Sorry for the confusion!

Not quite. It's only intended to hide suggestions for new features that aren't available for all versions supported by a project.

I was afraid of the potential for unexpected compatibility work to do but maybe you're right. The tests matrix just got enormous though. Do we need to check functional tests with each py-version ? It seems like we should but this would add maybe 5mn per pipeline.

Perhaps we could make one functional test for py-version <= 3.5? Instead of splitting it over multiple additional files, have one file test that all warnings which shouldn't be emitted on 3.5 or lower are not emitted? This goes against the idea of making functional tests for each individual checker, but might be a cleaner solution.

That's what I've done here 🙂 One test to make sure these warnings aren't emitted if py-version=3.5. There is no need to do much more than that. Tests for features that aren't fully supported just need to add py-version. E.g. for assignment expressions I set py-version=3.8. It will run just fine even when executed with Python 3.6.

[cdce8p]

IMO this PR is ready. Left a comment on the last open question: #5024 (comment)