From 43201f601bc55d8853c1582bd79ca331dbd7e317 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Mon, 1 Jul 2024 15:45:32 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=9A=20Re-enable=20pylint=20(#5377)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pylint has been disabled since the migration to Python 3.12. It's valuable to have as a backstop for simple programming mistakes, so this PR tries to re-enable it. **How to test** If the build passes, we did it! --- build-tools/github/validate | 1 + build-tools/github/validate-python | 19 ++++++++ requirements.txt | 3 +- tests/test_dynamo.py | 10 ++--- tests_e2e.py | 2 +- website/database.py | 70 +++++++++++++++--------------- website/dynamo.py | 4 +- website/user_activity.py | 2 +- 8 files changed, 66 insertions(+), 45 deletions(-) create mode 100755 build-tools/github/validate-python diff --git a/build-tools/github/validate b/build-tools/github/validate index 2269ffcde8c..0428097d7ef 100755 --- a/build-tools/github/validate +++ b/build-tools/github/validate @@ -7,6 +7,7 @@ scriptdir=$(cd $(dirname $0) && pwd) # TODOs about extracting by JW 2023-10-03 for PR #4574 $scriptdir/validate-typescript # TODO extract +$scriptdir/validate-python # TODO extract $scriptdir/validate-yaml # TODO extract if [[ "${1:-}" == "--all" ]]; then $scriptdir/validate-tests --all diff --git a/build-tools/github/validate-python b/build-tools/github/validate-python new file mode 100755 index 00000000000..6fb925361a8 --- /dev/null +++ b/build-tools/github/validate-python @@ -0,0 +1,19 @@ +#!/bin/bash +# Validate Python code +set -eu +scriptdir=$(cd $(dirname $0) && pwd) +cd $scriptdir/../.. + +echo "------> Check Python for problems using pylint" + +# Find all .py files to validate, except those in the 'prefixes' directory. +# Syntax: ( -o -o ... -prune ) +pyfiles=$(find . -not \( -path ./.\* -o -path ./prefixes -prune \) -name \*.py) + +# Get the system/library directories. pylint should not report any errors against those. +sysdirs=$(python3 -c 'import sys; print(",".join(p for p in sys.path if p))') + +pylint -E \ + --jobs 0 \ + --ignore-paths $sysdirs \ + $pyfiles diff --git a/requirements.txt b/requirements.txt index 74425cc7276..a1ba2d68eb6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,7 +12,7 @@ ruamel.yaml==0.18.6 docopt==0.6.2 pydantic==1.10.13 lazy==1.4 -awscli>=1.19.88 +awscli==1.32.77 PySumTypes==0.0.1 beautifulsoup4==4.9.3 regex==2021.8.28 @@ -34,5 +34,6 @@ doit==0.36.0 doit_watch>=0.1.0 uflash>=2.0.0 pyinstaller==6.3.0 +pylint==3.1.0 commonmark==0.9.1 check-jsonschema diff --git a/tests/test_dynamo.py b/tests/test_dynamo.py index 741a2703602..f1a6167215d 100644 --- a/tests/test_dynamo.py +++ b/tests/test_dynamo.py @@ -4,7 +4,7 @@ from unittest import mock from website import dynamo -from website.dynamo import Optional +from website.dynamo import OptionalOf class Helpers: @@ -262,10 +262,10 @@ def setUp(self): types={ 'id': str, 'sort': int, - 'x': Optional(int), - 'y': Optional(int), - 'm': Optional(int), - 'n': Optional(int), + 'x': OptionalOf(int), + 'y': OptionalOf(int), + 'm': OptionalOf(int), + 'n': OptionalOf(int), }, indexes=[ dynamo.Index( diff --git a/tests_e2e.py b/tests_e2e.py index 7d15002e0a5..61bccb8333d 100644 --- a/tests_e2e.py +++ b/tests_e2e.py @@ -411,7 +411,7 @@ def test_get_session_variables(self): self.assertIn('session', test_body) self.assertIn('session_id', test_body['session']) self.assertIn('test_session', test_body['session']) - self.assertEquals(test_body['session']['session_id'], session['id']) + self.assertEqual(test_body['session']['session_id'], session['id']) # WHEN getting session variables from the main environment body = self.get_data('/session_main') diff --git a/website/database.py b/website/database.py index 3f4817e11a6..bf41c15b4e1 100644 --- a/website/database.py +++ b/website/database.py @@ -37,7 +37,7 @@ from . import dynamo, auth from . import querylog -from .dynamo import DictOf, Optional, ListOf, SetOf, RecordOf, EitherOf +from .dynamo import DictOf, OptionalOf, ListOf, SetOf, RecordOf, EitherOf is_offline = getattr(sys, 'frozen', False) and hasattr(sys, '_MEIPASS') if is_offline: @@ -62,26 +62,26 @@ def only_in_dev(x): types=only_in_dev({ 'username': str, 'password': str, - 'email': Optional(str), - 'language': Optional(str), - 'keyword_language': Optional(str), + 'email': OptionalOf(str), + 'language': OptionalOf(str), + 'keyword_language': OptionalOf(str), 'created': int, - 'is_teacher': Optional(int), - 'verification_pending': Optional(str), + 'is_teacher': OptionalOf(int), + 'verification_pending': OptionalOf(str), 'last_login': int, - 'country': Optional(str), - 'birth_year': Optional(int), - 'gender': Optional(str), - 'heard_about': Optional(ListOf(str)), - 'prog_experience': Optional(str), - 'experience_languages': Optional(ListOf(str)), + 'country': OptionalOf(str), + 'birth_year': OptionalOf(int), + 'gender': OptionalOf(str), + 'heard_about': OptionalOf(ListOf(str)), + 'prog_experience': OptionalOf(str), + 'experience_languages': OptionalOf(ListOf(str)), 'epoch': int, - 'second_teacher_in': Optional(ListOf(str)), - 'classes': Optional(SetOf(str)), - 'teacher': Optional(str), - 'pair_with_teacher': Optional(int), - 'teacher_request': Optional(bool), - 'is_super_teacher': Optional(int) + 'second_teacher_in': OptionalOf(ListOf(str)), + 'classes': OptionalOf(SetOf(str)), + 'teacher': OptionalOf(str), + 'pair_with_teacher': OptionalOf(int), + 'teacher_request': OptionalOf(bool), + 'is_super_teacher': OptionalOf(int) }), indexes=[ dynamo.Index('email'), @@ -104,16 +104,16 @@ def only_in_dev(x): 'session': str, 'username': str, 'date': int, - 'hedy_choice': Optional(int), - 'public': Optional(int), + 'hedy_choice': OptionalOf(int), + 'public': OptionalOf(int), 'lang': str, 'level': int, 'code': str, 'adventure_name': str, 'name': str, 'username_level': str, - 'error': Optional(bool), - 'is_modified': Optional(bool) + 'error': OptionalOf(bool), + 'is_modified': OptionalOf(bool) }), indexes=[ dynamo.Index('username', sort_key='date', index_name='username-index'), @@ -135,11 +135,11 @@ def only_in_dev(x): 'link': str, 'date': int, 'name': str, - 'second_teachers': Optional(ListOf(RecordOf({ + 'second_teachers': OptionalOf(ListOf(RecordOf({ 'role': str, 'username': str, }))), - 'students': Optional(SetOf(str)), + 'students': OptionalOf(SetOf(str)), }), indexes=[ dynamo.Index('teacher'), @@ -164,13 +164,13 @@ def only_in_dev(x): 'date': int, 'creator': str, 'name': str, - 'classes': Optional(ListOf(str)), + 'classes': OptionalOf(ListOf(str)), 'level': EitherOf(str, int), # this might be a string or a int 'levels': ListOf(str), 'content': str, 'public': int, 'language': str, - 'formatted_content': Optional(str) + 'formatted_content': OptionalOf(str) }), indexes=[ dynamo.Index("creator"), @@ -206,7 +206,7 @@ def only_in_dev(x): 'id': str, 'name': str, 'popularity': int, - 'tagged_in': Optional(ListOf(RecordOf({ + 'tagged_in': OptionalOf(ListOf(RecordOf({ 'id': str, 'public': int, 'language': str @@ -225,7 +225,7 @@ def only_in_dev(x): SURVEYS = dynamo.Table(storage, "surveys", "id", types=only_in_dev({ 'id': str, - 'responses': Optional(DictOf({ + 'responses': OptionalOf(DictOf({ str: RecordOf({ 'answer': str, 'question': str @@ -287,17 +287,17 @@ def only_in_dev(x): 'from_teacher': bool })) }), - 'updated_by': Optional(str), - 'quiz_parsons_tabs_migrated': Optional(int) + 'updated_by': OptionalOf(str), + 'quiz_parsons_tabs_migrated': OptionalOf(int) })) ACHIEVEMENTS = dynamo.Table(storage, "achievements", partition_key="username", types=only_in_dev({ 'username': str, - 'achieved': Optional(ListOf(str)), - 'commands': Optional(ListOf(str)), - 'saved_programs': Optional(int), - 'run_programs': Optional(int) + 'achieved': OptionalOf(ListOf(str)), + 'commands': OptionalOf(ListOf(str)), + 'saved_programs': OptionalOf(int), + 'run_programs': OptionalOf(int) })) PUBLIC_PROFILES = dynamo.Table(storage, "public_profiles", partition_key="username", types=only_in_dev({ @@ -305,7 +305,7 @@ def only_in_dev(x): 'image': str, 'personal_text': str, 'agree_terms': str, - 'tags': Optional(ListOf(str)) + 'tags': OptionalOf(ListOf(str)) })) PARSONS = dynamo.Table(storage, "parsons", "id", types=only_in_dev({ diff --git a/website/dynamo.py b/website/dynamo.py index 0eeeb8352b6..e91c854a5fa 100644 --- a/website/dynamo.py +++ b/website/dynamo.py @@ -1709,7 +1709,7 @@ def ensure_all(validator_dict): def is_valid(self, value): ... - def __str__(self, value): + def __str__(self): ... @@ -1762,7 +1762,7 @@ def __str__(self): return f'matches {self.fn}' -class Optional(Validator): +class OptionalOf(Validator): """Validator which matches either None or an inner validator.""" def __init__(self, inner): diff --git a/website/user_activity.py b/website/user_activity.py index 50db6b7d604..ea1560e93a0 100644 --- a/website/user_activity.py +++ b/website/user_activity.py @@ -1,5 +1,5 @@ -import gettext +from flask_babel import gettext import os from flask import make_response, request, session