Fix pylint problems

[vojtechjelinek]

The goal of this issue is to enable some pylint rules that we disabled while doing the Python 3 migration.

Steps to follow to create a PR:

1. Ask @seanlip to assign you to one of the items below
2. Remove the assigned item from the .pylintrc
3. Run python -m scripts.linters.run_lint_checks --only-check-file-extensions=py --path=core --verbose to check existing issues in the codebase. (This can take 3-5 mins mins to complete.)
4. If the above script takes too much time or you have a slow machine then consider pushing the code to GitHub and GitHub actions will run these checks for you.
5. Fix all the issues found by running the above command.
6. Commit all the changes and create a PR.

Example PRs: #14474

Remove some disables from the .pylintrc:

• cyclic-import
• consider-using-with @m0nax5 (also, for reference, see previous PR by @anjaist Fix part of #12912: Reintroduce consider-using-with linter #18139)

Refactor custom validators that are not working correctly:

• Remove ExplicitKeywordArgsChecker #16366 (assignments tracked in linked issue)

Decided not to introduce:

• consider-using-dict-items @mjsumpter
• consider-using-f-string
• super-with-arguments @jrvald
• unnecessary-pass
• arguments-renamed
• import-outside-toplevel

[dushyantbhardwaj9]

Hey @vojtechjelinek, Is this still open?

[vojtechjelinek]

@dushyantbhardwaj9 Yes

[Manan-Rathi]

Hi @vojtechjelinek! Could you kindly assign me no-else-continue and no-else-raise?

[DubeySandeep]

Hi @Manan-Rathi, I've assigned you to no-else-continue and no-else-raise!

[Manan-Rathi]

Hi @Manan-Rathi, I've assigned you to no-else-continue and no-else-raise!

Should I create two separate PRs or just one?

[DubeySandeep]

@Manan-Rathi Create one PR with all the changes i.e, enabling both the rules and fixing existing lint issues in the codebase. (Considering the number of changed files in the PR won't be > 50)

[IamEzio]

Hi @vojtechjelinek @DubeySandeep ! Can I be assigned to deprecated-method ?

[DubeySandeep]

@IamEzio I've assigned you to deprecated-method!

[IamEzio]

Hey @DubeySandeep Can I also work on consider-using-in ?

[rohittiwari761]

Hey @vojtechjelinek Can you please assign me unnecessary-pass?

[vojtechjelinek]

@rohittiwari761 Hey, can you send the errors that show up when you enable unnecessary-pass? I just realized that we might not want to enable this one.

[vojtechjelinek]

@IamEzio Done.

[Manan-Rathi]

Hi @vojtechjelinek. Kindly assign me useless-object-inheritance.

[raghavluthra20]

Hey @vojtechjelinek Can I work on consider-using-f-string?

[vojtechjelinek]

@raghavluthra20 Please wait a bit as f-strings are not used in our codebase and I need to check with the rest of the core maintainers. Feel free to pick different rule for now.

[Dhruv454000]

hi @vojtechjelinek can i be assigned to super-with-arguments

[vojtechjelinek]

@Dhruv454000 Done.

[PietroFracCab]

@PietroFracCab Are you still working on the cyclic-imports?

Yes. I was out in vacation. should be done until next week.

[U8NWXD]

De-assigning @richard-johansson @PietroFracCab due to inactivity. Please let me know if you're still working on this!

[rahulsurwade08]

Hey @vojtechjelinek @U8NWXD I would love to work on this issue. How can I get started?

[U8NWXD]

@rahulsurwade08 see the instructions at the top of this issue. If you still have questions, please be more specific about what you find confusing

[josieecook]

@U8NWXD Hello! Could I please be assigned to cyclic-import? Thank you!

[Priyansh61]

Hi! @josieecook Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.

[anjaist]

Hello @U8NWXD, I'd like to be assigned consider-using-with.
Here are the files that would be affected:

core/controllers/profile_test.py
core/controllers/editor_test.py
core/domain/exp_services_test.py

Example change:
before:

zf_saved = zipfile.ZipFile(io.BytesIO(data.body))
assert zf_saved[...]

after:

with zipfile.ZipFile(io.BytesIO(data.body)) as zf_saved:
    assert zs_saved[...]

Additionally, I'd add a single # pylint: disable=consider-using-with line here, as the usage of this function is always using a with statement and we do only casting here. I'm not sure if there is an alternative way of doing this apart from changing the way this utils.open_file func is used.

[adityauwu]

Hi @vojtechjelinek ,
M new to open source and I have tried to disable from the .pylintrc : cyclic-import

Steps):

1. Removed the cyclic-import from the .pylintrc file
   2)Ran python -m scripts.linters.pre_commit_linter --only-check-file-extensions=py --path=core --verbose to check
   3)Passed without any errors

Please refer the attached below:

[seanlip]

@adityauwu You're welcome to work on this if you like. Do you want to be assigned? If so, please say that explicitly next time. Also please ensure you've followed all the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia bedore opening a PR. Thanks!

[adityauwu]

Hi @seanlip , Yes please assign it to me, I have created a PR please review and guide me through the same! Thanks

[m0nax5]

Hi @seanlip ! I would have liked to be assigned consider-using-with so I tried following the steps you gave to create a PR but I ran into an issue.

• I removed consider-using-with from the .pylintrc
• I tried running python -m scripts.linters.pre_commit_linter --only-check-file-extensions=py --path=core --verbose but I faced the following error /usr/bin/python: No module named scripts.linters.pre_commit_linter
• So I checked my code base and there is no pre_commit_linter directory inside scripts/linters/

I there anything that has changed in the code base since your original post ? Has it anything to do with the scripts/pre_commit_hook_test.py file ? Should I run another command to check for issues ?
Thanks for your help !

[seanlip]

@m0nax5 I think pre_commit_linter got renamed to run_lint_checks. See https://github.com/oppia/oppia/wiki/Lint-Checks. I've updated the issue description accordingly, thanks for raising this.

Also deassigning @adityauwu due to lack of activity.

[m0nax5]

@seanlip thanks I was able to run the linter checks ! But I also had to manually install esprima by running pip install esprima for it to work. Maybe it should be added to the dependencies installed during the initial installation of the Oppia repository ?

Also could you please assign me consider-using-with ? My code is ready to create a PR. Thanks !

[seanlip]

Thanks @m0nax5, I've assigned you. Could you please file an issue about the dependency issue you mentioned and steps to reproduce it on our wiki repo: github.com/oppia/oppia-web-developer-docs -- as well as your suggested fix? Thanks!

[m0nax5]

I won’t be filing an issue since I wasn’t able to reproduce the bug, even though I tried on another machine. I checked and the dependency I was missing was supposed to have been installed since it is in requirements_dev.in in line 9 : esprima==4.0.1, and 4.0.1 is the same version that I had to download manually. The same happened to me later with other libraries so I guess something just went wrong during the installation process on my machine.

[Felix-Mauyon]

Hi @seanlip

I ran this: python -m scripts.linters.run_lint_checks --only-check-file-extensions=py --path=core --verbose
And it showed this

Does this mean this issue has been fixed?

[m0nax5]

Hi @Felix-Mauyon ! If I may, did you chose an item to remove from the .pylintrc file like mentioned in step 2 ? If not it's normal that the checks should all pass : it's only when you remove one of the mentioned lines that a lint check is added, which may then bring about new failures (the ones that this issue aims to fix). Hope this helps !

[seanlip]

Confirming @m0nax5's explanation is spot-on (thanks @m0nax5)!

[Felix-Mauyon]

Thanks @m0nax5

If I understand you well, I am supposed to choose one of the item from step 2 and remove that line from the .pylintrc and rerun this:

python -m scripts.linters.run_lint_checks --only-check-file-extensions=py --path=core --verbose in the hope of getting a new failure. Is the failure a confirmation that it worked or I am supposed to do something about the failure as well?

[Victor-ID]

Hi @seanlip, can I be assigned to fix the consider-using-with rule?

[seanlip]

@Victor-ID I think @m0nax5 is working on this, but that PR has been stagnant for a while.

@m0nax5 are you working on this still, or would you like someone else to take it up?
@Victor-ID There is a PR open in #20134 that seems blocked. Can you help figure out what the issue is? If so then you might be able to take this up or collaborate with @m0nax5 on getting that PR merged.

Thanks!

[m0nax5]

Hi @seanlip, I've been stuck on that issue for a while now and I haven't had as much time to work on it, it would be great if @Victor-ID could have a look at it, and even take it up if he would like to. Also if @Victor-ID you find a solution to it and would like me to help with the code I've submitted we could collaborate on it too, whichever way you prefer is fine by me !