oppia: Fix pylint problems

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:

Ask @seanlip to assign you to one of the items below
Remove the assigned item from the .pylintrc
Run python -m scripts.linters.pre_commit_linter --only-check-file-extensions=py --path=core --verbose to check existing issues in the codebase. (This can take 3-5 mins mins to complete.)
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.
Fix all the issues found by running the above command.
Commit all the changes and create a PR.

Example PRs: #14474

Remove some disables from the .pylintrc:

cyclic-import
consider-using-with (unclaimed: for reference, see previous PR by @anjaist #18139)

Refactor custom validators that are not working correctly:

#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

About this issue

Original URL
State: open
Created 3 years ago
Comments: 122 (70 by maintainers)

Commits related to this issue

Fix parts of #12912: Enables useless-object-inheritance (#14551) — committed to oppia/oppia by Manan-Rathi 2 years ago
Fix part of #12912: Enables `consider-using-in` pylint check (#14520) — committed to oppia/oppia by IamEzio 2 years ago
Fix part of #12912: Changes deprecated python methods (#14506) — committed to oppia/oppia by IamEzio 2 years ago
Fix parts of #12912: Enables `no-else-continue` and `no-else-raise` pylint checks (#14504) Co-authored-by: Manan Rathi <rathimanan27@gmail.com> — committed to oppia/oppia by Manan-Rathi 2 years ago
Fix part of #12912: Enables raise-missing-from pylint check (#14719) — committed to oppia/oppia by ananth243 2 years ago
Fix part of #12912: Enables 'missing-raises-doc' and 'cyclic-import' pylint checks (#14772) — committed to oppia/oppia by AyushJ7 2 years ago
Fix part of #12912. Removing 'super-with-arguments' and fixing all files with errors on super() — committed to jrvald/oppia by jrvald 2 years ago
Fix part of #12912: Enable pylint 'super-with-arguments' (#15812) * Fix part of #12912. Removing 'super-with-arguments' and fixing all files with errors on super() * Re-added the 'webapp2.Response... — committed to oppia/oppia by jrvald 2 years ago

Most upvoted comments

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.

anjaist on Apr 20, 2023

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.

Priyansh61 on Mar 24, 2023

@richard-johansson Done.

vojtechjelinek on Nov 23, 2022

@prakhar1144 Are you working on this?

@richard-johansson I think you can work on consider-using-with if @prakhar1144 doesn’t reply.

vojtechjelinek on Nov 22, 2022

@PietroFracCab Done, please create a PR.

vojtechjelinek on Nov 10, 2022

@PietroFracCab Hey, sorry for delay in reply, can you please give me comment access to your doc? I have some suggestions.

vojtechjelinek on Oct 20, 2022

@prakhar1144 Done.

@anurag629 Please pick a different issue, everything is assigned here.

vojtechjelinek on Oct 2, 2022

Hey, @vojtechjelinek assign me if any issue is available and also tell me how to proceed with same.

Thanks

anurag629 on Oct 1, 2022

@Shaman20 That one might be a bit complicated, I recommend picking one from the first list.

vojtechjelinek on Aug 1, 2022

@jrvald both

vojtechjelinek on Jul 28, 2022

@mjsumpter I’ve assigned you consider-using-dict-items

vojtechjelinek on Jul 19, 2022

@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)

DubeySandeep on Dec 28, 2021

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

DubeySandeep on Dec 28, 2021