Fix oppia#13239 and oppia#13171: Adding type annotations to root file…

…s and fix constants errors with mypy (oppia#13269)
brianrodri · Jul 7, 2021 · 186435d · 186435d
1 parent 7f5757f
commit 186435d
Show file tree

Hide file tree

Showing 22 changed files with 253 additions and 172 deletions.
diff --git a/.isort.cfg b/.isort.cfg
@@ -2,6 +2,6 @@
 force_single_line=true
 force_sort_within_sections=true
 ignore_whitespace=true
-known_third_party=apache_beam,backports.functools_lru_cache,browsermobproxy,cloudstorage,contextlib2,elasticsearch,firebase_admin,google.api_core,google.appengine,google.cloud,google.protobuf,mapreduce,mock,mutagen,pipeline,pkg_resources,psutil,pylatexenc,pylint,requests,requests_mock,selenium,six,skulpt,webapp2,webapp2_extras,webtest,yaml
+known_third_party=apache_beam,backports.functools_lru_cache,browsermobproxy,cloudstorage,contextlib2,elasticsearch,firebase_admin,google.api_core,google.appengine,google.cloud,google.protobuf,mapreduce,mock,mutagen,pipeline,pkg_resources,psutil,pylatexenc,pylint,requests,requests_mock,selenium,six,skulpt,typing,webapp2,webapp2_extras,webtest,yaml
 line_length=80
 sections=FUTURE,STDLIB,FIRSTPARTY,THIRDPARTY,LOCALFOLDER
diff --git a/.pylintrc b/.pylintrc
@@ -58,6 +58,7 @@ bad-functions=apply,input
 [FORMAT]
 
 max-line-length=80
+ignore-long-lines=^.*#\stype:.*$|^\s*(# )?<?https?://\S+>?$
 
 indent-string='    '
 

diff --git a/android_validation_constants_test.py b/android_validation_constants_test.py
@@ -28,6 +28,7 @@ class AndroidValidationConstantsTest(test_utils.GenericTestBase):
     """Tests verifying the character limits."""
 
     def test_that_character_limits_in_both_files_are_equal(self):
+        # type: () -> None
         self.assertEqual(
             android_validation_constants.MAX_CHARS_IN_ABBREV_TOPIC_NAME,
             constants.MAX_CHARS_IN_ABBREV_TOPIC_NAME)
@@ -57,6 +58,7 @@ def test_that_character_limits_in_both_files_are_equal(self):
             constants.MAX_CHARS_IN_MISCONCEPTION_NAME)
 
     def test_exploration_constants_in_both_files_are_equal(self):
+        # type: () -> None
         interaction_ids_in_constants = []
         language_ids_in_constants = []
         constants_interactions_list = (

diff --git a/appengine_config.py b/appengine_config.py
@@ -25,6 +25,8 @@
 from google.appengine.ext import vendor
 import pkg_resources
 
+from typing import Any, Text # isort:skip # pylint: disable=unused-import
+
 # Root path of the app.
 ROOT_PATH = os.path.dirname(__file__)
 THIRD_PARTY_PATH = os.path.join(ROOT_PATH, 'third_party')
@@ -44,7 +46,7 @@
     PIL_PATH = os.path.join(OPPIA_TOOLS_PATH, 'Pillow-6.2.2')
     if not os.path.isdir(PIL_PATH):
         raise Exception('Invalid path for oppia_tools library: %s' % PIL_PATH)
-    sys.path.insert(0, PIL_PATH)
+    sys.path.insert(0, PIL_PATH) # type: ignore[arg-type]
 
 
 # Google App Engine (GAE) uses its own virtual environment that sets up the
@@ -80,19 +82,21 @@
 # It should be fine to ignore this, see https://stackoverflow.com/a/47494229
 # and https://github.com/urllib3/urllib3/issues/1138#issuecomment-290325277.
 import requests # isort:skip  pylint: disable=wrong-import-position, wrong-import-order
-requests.packages.urllib3.disable_warnings(
-    requests.packages.urllib3.contrib.appengine.AppEnginePlatformWarning
+requests.packages.urllib3.disable_warnings( # type: ignore[no-untyped-call]
+    requests.packages.urllib3.contrib.appengine.AppEnginePlatformWarning # type: ignore[attr-defined]
 )
 
 
 class MockDistribution(python_utils.OBJECT):
     """Mock distribution object for the monkeypatching function."""
 
     def __init__(self, version):
+        # type: (Text) -> None
         self.version = version
 
 
 def monkeypatched_get_distribution(distribution_name):
+    # type: (Text) -> Any
     """Monkeypatched version of pkg_resources.get_distribution.
 
     This approach is inspired by the discussion at:

diff --git a/appengine_config_test.py b/appengine_config_test.py
@@ -23,11 +23,14 @@
 from core.tests import test_utils
 import pkg_resources
 
+from typing import Text # isort:skip # pylint: disable=unused-import
+
 
 class AppengineConfigTests(test_utils.GenericTestBase):
     """Test the appengine config mock methods."""
 
     def _mock_get_distribution_which_raises_error(self, distribution_name):
+        # type: (Text) -> None
         """Mock function for pkg_resources.get_distribution().
 
         Args:
@@ -42,6 +45,7 @@ def _mock_get_distribution_which_raises_error(self, distribution_name):
         raise pkg_resources.DistributionNotFound(distribution_name, 'tests')
 
     def test_monkeypatched_get_distribution_for_google_cloud_tasks(self):
+        # type: () -> None
         """Test that the monkey-patched get_distribution() method returns an
         object with a suitable version string for google-cloud-tasks.
         """
@@ -54,6 +58,7 @@ def test_monkeypatched_get_distribution_for_google_cloud_tasks(self):
         self.assertEqual(mock_distribution.version, '1.5.0')
 
     def test_monkeypatched_get_distribution_for_google_cloud_translate(self):
+        # type: () -> None
         """Test that the monkey-patched get_distribution() method returns an
         object with a suitable version string for google-cloud-tasks.
         """
@@ -66,10 +71,11 @@ def test_monkeypatched_get_distribution_for_google_cloud_translate(self):
         self.assertEqual(mock_distribution.version, '2.0.1')
 
     def test_monkeypatched_get_distribution_behaves_like_existing_one(self):
+        # type: () -> None
         """Test that the monkey-patched get_distribution() method returns an
         object with a suitable version string for google-cloud-tasks.
         """
-        assert_raises_regexp_context_manager = self.assertRaisesRegexp(
+        assert_raises_regexp_context_manager = self.assertRaisesRegexp( # type: ignore[no-untyped-call]
             pkg_resources.DistributionNotFound, 'invalid-lib')
         with self.swap(
             appengine_config, 'old_get_distribution',

diff --git a/constants.py b/constants.py
@@ -25,8 +25,11 @@
 
 import python_utils
 
+from typing import Any, Dict, Text, TextIO # isort:skip # pylint: disable=unused-import
+
 
 def parse_json_from_js(js_file):
+    # type: (TextIO) -> Dict[Text, Any]
     """Extracts JSON object from JS file.
 
     Args:
@@ -40,10 +43,12 @@ def parse_json_from_js(js_file):
     json_start = text_without_comments.find('{\n')
     # Add 1 to index returned because the '}' is part of the JSON object.
     json_end = text_without_comments.rfind('}') + 1
-    return json.loads(text_without_comments[json_start:json_end])
+    json_dict = json.loads(text_without_comments[json_start:json_end]) # type: Dict[Text, Any]
+    return json_dict
 
 
 def remove_comments(text):
+    # type: (Text) -> Text
     """Removes comments from given text.
 
     Args:
@@ -55,14 +60,20 @@ def remove_comments(text):
     return re.sub(r'  //.*\n', r'', text)
 
 
-class Constants(dict):
+class Constants(dict): # type: ignore[type-arg]
     """Transforms dict to object, attributes can be accessed by dot notation."""
 
-    __getattr__ = dict.__getitem__
+    def __setattr__(self, name, value):
+        # type: (Text, Any) -> None
+        self[name] = value
+
+    def __getattr__(self, name):
+        # type: (Text) -> Any
+        return self[name]
 
 
-with python_utils.open_file(os.path.join('assets', 'constants.ts'), 'r') as f:
+with python_utils.open_file(os.path.join('assets', 'constants.ts'), 'r') as f: # type: ignore[no-untyped-call]
     constants = Constants(parse_json_from_js(f))  # pylint:disable=invalid-name
 
-with python_utils.open_file('release_constants.json', 'r') as f:
+with python_utils.open_file('release_constants.json', 'r') as f: # type: ignore[no-untyped-call]
     release_constants = Constants(json.loads(f.read()))  # pylint:disable=invalid-name
diff --git a/constants_test.py b/constants_test.py
@@ -28,19 +28,22 @@
 class ConstantsTests(test_utils.GenericTestBase):
 
     def test_constants_file_is_existing(self):
+        # type: () -> None
         """Test if the constants file is existing."""
         self.assertTrue(os.path.isfile(os.path.join(
             'assets', 'constants.ts')))
 
     def test_constants_file_contains_valid_json(self):
+        # type: () -> None
         """Test if the constants file is valid json file."""
-        with python_utils.open_file(
+        with python_utils.open_file( # type: ignore[no-untyped-call]
             os.path.join('assets', 'constants.ts'), 'r') as f:
             json = constants.parse_json_from_js(f)
             self.assertTrue(isinstance(json, dict))
             self.assertEqual(json['TESTING_CONSTANT'], 'test')
 
     def test_difficulty_values_are_matched(self):
+        # type: () -> None
         """Tests that the difficulty values and strings are matched in the
         various constants.
         """
@@ -58,6 +61,7 @@ def test_difficulty_values_are_matched(self):
                 constants.constants.SKILL_DIFFICULTY_EASY])
 
     def test_constants_and_feconf_are_consistent(self):
+        # type: () -> None
         """Test if constants that are related are consistent between feconf and
         constants.js.
         """
@@ -68,10 +72,11 @@ def test_constants_and_feconf_are_consistent(self):
         self.assertEqual(len(constants.constants.SYSTEM_USER_IDS), 2)
 
     def test_all_comments_are_removed_from_json_text(self):
+        # type: () -> None
         """Tests if comments are removed from json text."""
         dummy_constants_filepath = os.path.join(
             feconf.TESTS_DATA_DIR, 'dummy_constants.js')
-        with python_utils.open_file(dummy_constants_filepath, 'r') as f:
+        with python_utils.open_file(dummy_constants_filepath, 'r') as f: # type: ignore[no-untyped-call]
             actual_text_without_comments = constants.remove_comments(f.read())
             expected_text_without_comments = (
                 'var dummy_constants = {\n'
@@ -88,6 +93,7 @@ def test_all_comments_are_removed_from_json_text(self):
                 actual_text_without_comments, expected_text_without_comments)
 
     def test_language_constants_are_in_sync(self):
+        # type: () -> None
         """Test if SUPPORTED_CONTENT_LANGUAGES and SUPPORTED_AUDIO_LANGUAGES
         constants have any conflicting values.
         """

diff --git a/core/controllers/payload_validator.py b/core/controllers/payload_validator.py
@@ -60,7 +60,7 @@ def validate(handler_args, handler_args_schemas, allowed_extra_args):
                 continue
 
         try:
-            normalized_value[arg_key] = schema_utils.normalize_against_schema( # type: ignore[no-untyped-call] # pylint: disable=line-too-long
+            normalized_value[arg_key] = schema_utils.normalize_against_schema(
                 handler_args[arg_key], arg_schema['schema'])
         except Exception as e:
             errors.append(

diff --git a/feconf.py b/feconf.py
@@ -25,6 +25,8 @@
 
 from constants import constants
 
+from typing import Dict, Text # isort:skip # pylint: disable=unused-import
+
 # The datastore model ID for the list of featured activity references. This
 # value should not be changed.
 ACTIVITY_REFERENCE_LIST_FEATURED = 'featured'
@@ -336,14 +338,14 @@
         'content': {},
         'default_outcome': {}
     }
-}
+} # type: Dict[Text, Dict[Text, Dict[Text, Text]]]
 # Default written_translations dict for a default state template.
 DEFAULT_WRITTEN_TRANSLATIONS = {
     'translations_mapping': {
         'content': {},
         'default_outcome': {}
     }
-}
+} # type: Dict[Text, Dict[Text, Dict[Text, Text]]]
 # The default content text for the initial state of an exploration.
 DEFAULT_INIT_STATE_CONTENT_STR = ''
 
@@ -422,6 +424,7 @@
 
 
 def get_empty_ratings():
+    # type: () -> Dict[Text, int]
     """Returns a copy of the empty ratings object.
 
     Returns:

diff --git a/main.py b/main.py
@@ -76,7 +76,9 @@
 import webapp2
 from webapp2_extras import routes
 
-transaction_services = models.Registry.import_transaction_services()
+from typing import Any, Dict, Optional, Text, Type # isort:skip # pylint: disable=unused-import
+
+transaction_services = models.Registry.import_transaction_services() # type: ignore[no-untyped-call]
 
 # Suppress debug logging for chardet. See https://stackoverflow.com/a/48581323.
 # Without this, a lot of unnecessary debug logs are printed in error logs,
@@ -89,18 +91,20 @@ class FrontendErrorHandler(base.BaseHandler):
 
     REQUIRE_PAYLOAD_CSRF_CHECK = False
 
-    @acl_decorators.open_access
+    @acl_decorators.open_access # type: ignore[misc]
     def post(self):
+        # type: () -> None
         """Records errors reported by the frontend."""
         logging.error('Frontend error: %s' % self.payload.get('error'))
-        self.render_json(self.values)
+        self.render_json(self.values) # type: ignore[no-untyped-call]
 
 
 class WarmupPage(base.BaseHandler):
     """Handles warmup requests."""
 
-    @acl_decorators.open_access
+    @acl_decorators.open_access # type: ignore[misc]
     def get(self):
+        # type: () -> None
         """Handles GET warmup requests."""
         pass
 
@@ -110,30 +114,32 @@ class HomePageRedirectPage(base.BaseHandler):
     redirect them appropriately.
     """
 
-    @acl_decorators.open_access
+    @acl_decorators.open_access # type: ignore[misc]
     def get(self):
-        if self.user_id and user_services.has_fully_registered_account(
+        # type: () -> None
+        if self.user_id and user_services.has_fully_registered_account( # type: ignore[no-untyped-call]
                 self.user_id):
-            user_settings = user_services.get_user_settings(
-                self.user_id)
+            user_settings = user_services.get_user_settings(self.user_id) # type: ignore[no-untyped-call]
             default_dashboard = user_settings.default_dashboard
             if default_dashboard == constants.DASHBOARD_TYPE_CREATOR:
                 self.redirect(feconf.CREATOR_DASHBOARD_URL)
             else:
                 self.redirect(feconf.LEARNER_DASHBOARD_URL)
         else:
-            self.render_template('splash-page.mainpage.html')
+            self.render_template('splash-page.mainpage.html') # type: ignore[no-untyped-call]
 
 
 class SplashRedirectPage(base.BaseHandler):
     """Redirect the old splash URL, '/splash' to the new one, '/'."""
 
-    @acl_decorators.open_access
+    @acl_decorators.open_access # type: ignore[misc]
     def get(self):
+        # type: () -> None
         self.redirect('/')
 
 
 def get_redirect_route(regex_route, handler, defaults=None):
+    # type: (Text, Type[base.BaseHandler], Optional[Dict[Any, Any]]) -> routes.RedirectRoute
     """Returns a route that redirects /foo/ to /foo.
 
     Warning: this method strips off parameters after the trailing slash. URLs
@@ -156,6 +162,7 @@ def get_redirect_route(regex_route, handler, defaults=None):
 
 
 def authorization_wrapper(self, *args, **kwargs):
+    # type: (Any, *Any, **Any) -> None
     """This request handler wrapper only admits internal requests from
     taskqueue workers. If the request is invalid, it leads to a 403 Error page.
     """
@@ -169,6 +176,7 @@ def authorization_wrapper(self, *args, **kwargs):
 
 
 def ui_access_wrapper(self, *args, **kwargs):
+    # type: (Any, *Any, **Any) -> None
     """This request handler wrapper directly serves UI pages
     for MapReduce dashboards.
     """
@@ -905,4 +913,4 @@ def ui_access_wrapper(self, *args, **kwargs):
 app = transaction_services.toplevel_wrapper(  # pylint: disable=invalid-name
     webapp2.WSGIApplication(URLS, debug=feconf.DEBUG))
 
-firebase_auth_services.establish_firebase_connection()
+firebase_auth_services.establish_firebase_connection() # type: ignore[no-untyped-call]
diff --git a/main_cron.py b/main_cron.py
@@ -25,7 +25,7 @@
 import webapp2
 
 
-transaction_services = models.Registry.import_transaction_services()
+transaction_services = models.Registry.import_transaction_services() # type: ignore[no-untyped-call]
 
 # Register the URLs with the classes responsible for handling them.
 URLS = [

diff --git a/main_taskqueue.py b/main_taskqueue.py
@@ -25,7 +25,7 @@
 import webapp2
 
 
-transaction_services = models.Registry.import_transaction_services()
+transaction_services = models.Registry.import_transaction_services() # type: ignore[no-untyped-call]
 
 # Register the URLs with the classes responsible for handling them.
 URLS = [

diff --git a/mypy_requirements.txt b/mypy_requirements.txt
@@ -1,4 +1,13 @@
 # TODO(#13131): Merge this file into requirements.txt after python3 migration.
 # These packages are python3 packages to be used by MyPy.
 mypy==0.902
+types-backports==0.1.3
+types-bleach==3.3.3
+types-certifi==0.1.4
+types-enum34==0.1.8
+types-futures==0.1.6
+types-protobuf==0.1.13
 types-PyYAML==0.1.6
+types-redis==3.5.4
+types-requests==2.25.0
+types-six==0.1.7
diff --git a/requirements.in b/requirements.in
@@ -37,3 +37,4 @@ googleapis_common_protos==1.52.0
 webapp2==3.0.0b1
 elasticsearch==7.10.0
 mailchimp3==3.0.14
+typing==3.7.4.3