Add unspecified-encoding checker #3826

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	✨ New feature

Description

This adds an unspecified-encoding checker that adds a warning whenever open() is called without an explicit encoding argument.
This closes #3826.

I stumbled upon this issue today while I experienced the importance of adding encoding to open(). This is my first contribution to pylint so please let me know if anything needs to be changed or improved!

[Pierre-Sassoulas]

Thank you for handling this issue. This looks great. I made some small comments but this is definitely going in 2.10.0 :).

[DanielNoord]

@Pierre-Sassoulas I merged all your suggestions + added checks for b mode, which does not need an encoding.

I do not really have any experience with other open methods. Perhaps you can add some additional test cases your self, or give me a link to some documentation on open methods you would like to see added.

[Pierre-Sassoulas]

I think we should handle io.open, it's just an alias, but also io.open_code which is implicitly "rb" and make sure that we don't have false positive for it.

[DanielNoord]

Added the check for io.open and io.open_code.

However, the test suite is failing on some files which have not been edited by these commits while it is up to date with the main branch (besides the latest commit to issue forms).
Not sure what is going wrong here. Should I fix that in this PR? I don't immediately know where these errors are coming from.

[Pierre-Sassoulas]

Not sure what is going wrong here.

Well what happened is you discovered style problem inside pylint with your new checker 😄 The new problem in pylint should be fixed in this merge request, we can discuss them here. Maybe it'll fix strange bug that happened only on some particular OS 🎉

Or it can be problem in existing functional tests that are now detected. You can disable those new messages in the existing tests so there is no change in the expected output, or update the output if you think this make more sense.

[Pierre-Sassoulas]

👍

[DanielNoord]

Added some additional encoding's to open() calls within the test-suite.
Sorry for taking so much of your time. For some reason the test-suite is really struggling to provide me some clear feedback on my own machine. Keep finding new errors after running the GitHub action that I did not clearly see in the output on my own machine...

Edit: Found some additional errors.. Back to the drawing board I guess, lol

[Pierre-Sassoulas]

Always happy to help :) Also don't stress out, this won't be merged before 2.10, so not before 2.9.6 is out 🙂

Regarding the latest change to fix the tests, it looks most of the changes are in functional tests. Please don't modify the functional test too much, you can disable the new checker with pylint: disable=unspecified-encoding at the top of the file (avoid creating a newline as the expected output will change completely if you add a line). The formatting changes seems like they are automated by your IDE and need to be reverted as having strange formattign is also part of what we're testing (j+1;lambda ... for example). Sorry for not being clearer in my earlier comment.

[DanielNoord]

Ah shit, I just changed some of the tests to include encodings and fixed line-numbering in the test data. Do you want me to remove those and add the override? Or is adding encodings to fix the errors also fine?
For me (Mac) the tests are also throwing a number of c-extension-no-member, no-member and no-name-in-module errors. Are those also related to the changes introduced by this PR? I don't think so, right?

[Pierre-Sassoulas]

A mix of disabling, changing the code and checking that the warning is raised is fine, it should not be one sided though, more like a third of each. (To be faire disable or checking that it's raised is an easier change but you already did the work of changing the code so... 😄)

I don't think the test suite run in the CI for mac right now as we have linux and windows only. So, its possible that we have some existing problem on mac. We supposed mac/linux were close enough and not worth the added pipeline time. Apparently it's not. You can check on the master branch for the test that are already failing while we fix this.

[DanielNoord]

Hmm, after fixing my formatter the tests are now also raising a number of issues with the code of pylint itself.
I guess open() is used often, who would have thought... :)

I would be very glad to fully fix all errors, but this won't be done tonight. Seeing as pylint has such a good test coverage already, I would feel bad if my contribution would lead to an increase in skipped checks. I will try and group some of the changes in similar commits for easier review. We can always squash those before the final merge.

[cdce8p]

What a first PR 🔥 I like it a lot! Did a test run and it already worked beautifully.
As this will go in 2.10, there is still some time. So you don't need to hurry.

[DanielNoord]

Almost made it work. Again, sorry for taking so much of your time...

Action at personal repo shows that the PR succeeds for CPython 3.8 =<, but fails for 3.7, 3.6 and pypy. I thought appending _py38 would make it automatically skip for 3.6 and 3.7 but I guess it is not that simple.
Furthermore, it is only one line that is failing (line 34) as io.open_code() was only added in 3.8. Perhaps there is a way to only make it skip that line for 3.6 and 3.7?

[cdce8p]

You would need to create a file like this one: generic_alias_typing.rc. With the same name as the test file, only rc as extension.

One more thing, I would appreciate if you wouldn't force push all your new changes. That makes it really difficult to review them. We can squash merge the PR in the end so it doesn't make a difference if there are 10+ commits. Usually a force push should only be used when doing a rebase to catch up with the latest changes on main.

[DanielNoord]

I'm really sorry! I thought you might want a commit history that represents a possible final history.
Will refrain from force pushing in the future!

[coveralls]

Coverage increased (+0.03%) to 92.18% when pulling 014c639 on DanielNoord:unspecified-encoding into c04f92e on PyCQA:main.

[cdce8p]

Looks good 🚀
Left some last comments

[Pierre-Sassoulas]

Haaa. I made a mistake I should have waited for 2.9.6 to be released before merging.

[cdce8p]

Haaa. I made a mistake I should have waited for 2.9.6 to be released before merging.

In that case, what do you think about releasing 2.10 next? As far as I've seen, there aren't any major bugfixes that need to go into 2.9.6. All can move to 2.10

[DanielNoord]

Also note that I didn't have time to merge @cdce8p's last comment

[Pierre-Sassoulas]

Also note that I didn't have time to merge @cdce8p's last comment

Don't worry, I applied the suggestion :)

I'm going to release 2.9.6 with #4765 and we'll start to merge the 2.10 features for the next release after that. I moved the remaining MR in 2.9.6 to 2.10.1.