🚚 Re-enable pylint (#5377)

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!
hedyorg · Jul 1, 2024 · 43201f6 · 43201f6
1 parent 0db9f8f
commit 43201f6
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 45 deletions.
diff --git 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
@@ -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: ( <condition> -o <condition> -o <condition> ... -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
@@ -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
@@ -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
@@ -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
@@ -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,25 +287,25 @@ 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({
                                    'username': str,
                                    '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
@@ -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
@@ -1,5 +1,5 @@
 
-import gettext
+from flask_babel import gettext
 import os
 from flask import make_response, request, session