Improving the dev workflow: Milestone 1.1.3 Custom pylint checks (#4967)

* Add local check * Basic setup * Fix basic lint errors * lint fix * more lint fix * more lint fix * lint fix core/controllers * partial lint fix core/domain * partial lint fix core/domain() * partial lint fix core/domain(2) * lint fix core/domain * lint fix core/platform * lint fix core/storage(1) * lint fix core/storage(2) * lint fix core/tests * lint fix core/jobs.py * lint fix scripts * more fixes * some more fixes * add tests * lint fix * Update returns * Review changes(1) * lint fix * minor fix * Address all review comments except returns * lint fix * fix yields * remove unnecessary returns * Fix backend tests * Address all review comments * Add doc utils * Add yields line * lint fix * Review changes * Review changes
oppia · May 25, 2018 · b00ad88 · b00ad88
1 parent ef7b206
commit b00ad88
Show file tree

Hide file tree

Showing 40 changed files with 870 additions and 98 deletions.
diff --git a/.pylintrc b/.pylintrc
@@ -50,27 +50,7 @@ ignore-imports=yes
 
 [MESSAGES CONTROL]
 
-# TODO(sll): Consider re-enabling the following checks:
-#    abstract-method
-#    arguments-differ
-#    broad-except
-#    duplicate-code
-#    fixme
-#    missing-docstring
-#    no-member
-#    no-self-use
-#    redefined-variable-type
-#    too-many-arguments
-#    too-many-boolean-expressions
-#    too-many-branches
-#    too-many-instance-attributes
-#    too-many-lines
-#    too-many-locals
-#    too-many-public-methods
-#    too-many-statements
-# and fix those issues.
-
-disable=consider-using-ternary,locally-disabled,locally-enabled,logging-not-lazy,abstract-method,arguments-differ,broad-except,duplicate-code,fixme,inconsistent-return-statements,invalid-name,len-as-condition,missing-docstring,no-else-return,no-member,no-self-use,not-context-manager,redefined-variable-type,too-many-arguments,too-many-boolean-expressions,too-many-branches,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-nested-blocks,too-many-statements,wrong-import-order
+disable=consider-using-ternary,locally-disabled,locally-enabled,logging-not-lazy,abstract-method,arguments-differ,broad-except,duplicate-code,fixme,inconsistent-return-statements,invalid-name,len-as-condition,missing-docstring,missing-raises-doc,multiple-constructor-doc,no-else-return,no-member,no-self-use,not-context-manager,redefined-variable-type,redundant-returns-doc,too-many-arguments,too-many-boolean-expressions,too-many-branches,too-many-instance-attributes,too-many-lines,too-many-locals,too-many-public-methods,too-many-nested-blocks,too-many-statements,wrong-import-order
 
 [REPORTS]
 

diff --git a/core/controllers/base.py b/core/controllers/base.py
@@ -244,7 +244,8 @@ def dispatch(self):
                     '%s: payload %s',
                     e, self.payload)
 
-                return self.handle_exception(e, self.app.debug)
+                self.handle_exception(e, self.app.debug)
+                return
 
         super(BaseHandler, self).dispatch()
 
@@ -440,7 +441,7 @@ def handle_exception(self, exception, unused_debug_mode):
         """Overwrites the default exception handler.
 
         Args:
-            exception: The exception that was thrown.
+            exception: Exception. The exception that was thrown.
             unused_debug_mode: bool. True if the web application is running
                 in debug mode.
         """
@@ -572,6 +573,9 @@ def is_csrf_token_valid(cls, user_id, token):
         Args:
             user_id: str. The user_id to validate the CSRF token against.
             token: str. The CSRF token to validate.
+
+        Returns:
+            bool. Whether the given CSRF token is valid.
         """
         try:
             parts = token.split('/')

diff --git a/core/domain/acl_decorators.py b/core/domain/acl_decorators.py
@@ -45,6 +45,10 @@ def test_can_play(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can play the given exploration.
         """
         if exploration_id in feconf.DISABLED_EXPLORATION_IDS:
             raise self.PageNotFoundException
@@ -70,6 +74,10 @@ def test_can_play(self, collection_id, **kwargs):
 
         Args:
             collection_id: str. The collection id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can play the given collection.
         """
         collection_rights = rights_manager.get_collection_rights(
             collection_id, strict=False)
@@ -94,6 +102,10 @@ def test_can_download(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can download the given exploration.
         """
         if exploration_id in feconf.DISABLED_EXPLORATION_IDS:
             raise base.UserFacingExceptions.PageNotFoundException
@@ -120,6 +132,10 @@ def test_can_view_stats(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can view the exploration stats.
         """
         if exploration_id in feconf.DISABLED_EXPLORATION_IDS:
             raise base.UserFacingExceptions.PageNotFoundException
@@ -144,6 +160,10 @@ def test_can_edit(self, collection_id, **kwargs):
 
         Args:
             collection_id: str. The collection id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can edit the collection.
         """
         if not self.user_id:
             raise base.UserFacingExceptions.NotLoggedInException
@@ -333,6 +353,10 @@ def test_can_access(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can view the exploration feedback.
         """
         if not self.user_id:
             raise base.UserFacingExceptions.NotLoggedInException
@@ -364,6 +388,10 @@ def test_can_rate(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can rate the exploration.
         """
         if (role_services.ACTION_RATE_ANY_PUBLIC_EXPLORATION in
                 self.user.actions):
@@ -384,6 +412,10 @@ def test_can_flag(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can flag the exploration.
         """
         if role_services.ACTION_FLAG_EXPLORATION in self.user.actions:
             return handler(self, exploration_id, **kwargs)
@@ -418,6 +450,10 @@ def test_can_edit(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can edit the exploration.
         """
         if not self.user_id:
             raise base.UserFacingExceptions.NotLoggedInException
@@ -446,6 +482,10 @@ def test_can_delete(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can delete the exploration.
         """
         if not self.user_id:
             raise base.UserFacingExceptions.NotLoggedInException
@@ -475,6 +515,11 @@ def test_can_suggest(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can make suggestions to an
+                exploration.
         """
         if (role_services.ACTION_SUGGEST_CHANGES_TO_EXPLORATION in
                 self.user.actions):
@@ -496,6 +541,11 @@ def test_can_publish(self, exploration_id, *args, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            *args: arguments.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can publish the exploration.
         """
         exploration_rights = rights_manager.get_exploration_rights(
             exploration_id, strict=False)
@@ -522,6 +572,10 @@ def test_can_publish_collection(self, collection_id, **kwargs):
 
         Args:
             collection_id: str. The collection id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can publish the collection.
         """
         collection_rights = rights_manager.get_collection_rights(
             collection_id)
@@ -547,6 +601,10 @@ def test_can_unpublish_collection(self, collection_id, **kwargs):
 
         Args:
             collection_id: str. The collection id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can unpublish the collection.
         """
         collection_rights = rights_manager.get_collection_rights(
             collection_id)
@@ -574,6 +632,11 @@ def test_can_modify(self, exploration_id, **kwargs):
 
         Args:
             exploration_id: str. The exploration id.
+            **kwargs: *. Keyword arguments.
+
+        Returns:
+            bool. Whether the user can modify the rights related to
+                an exploration.
         """
         exploration_rights = rights_manager.get_exploration_rights(
             exploration_id, strict=False)

diff --git a/core/domain/classifier_services.py b/core/domain/classifier_services.py
@@ -204,7 +204,7 @@ def _update_classifier_training_jobs_status(job_ids, status):
     """Checks for the existence of the model and then updates it.
 
     Args:
-        job_id: list(str). list of ID of the ClassifierTrainingJob domain
+        job_ids: list(str). list of ID of the ClassifierTrainingJob domain
             objects.
         status: str. The status to which the job needs to be updated.
 

diff --git a/core/domain/collection_domain.py b/core/domain/collection_domain.py
@@ -748,7 +748,7 @@ def get_next_exploration_id(self, completed_exp_ids):
            returns None.
 
         Args:
-            completed_exploration_ids: list(str). List of completed exploration
+            completed_exp_ids: list(str). List of completed exploration
                 ids.
 
         Returns:
@@ -789,7 +789,7 @@ def is_demo_collection_id(cls, collection_id):
         Args:
             collection_id: str. The id of the collection.
 
-        Returs:
+        Returns:
             bool. True if the collection is a demo else False.
         """
         return collection_id in feconf.DEMO_COLLECTIONS

diff --git a/core/domain/collection_services.py b/core/domain/collection_services.py
@@ -953,7 +953,7 @@ def compute_summary_of_collection(collection, contributor_id_to_add):
     object and return it.
 
     Args:
-        collection_id: str. ID of the collection.
+        collection: Collection. The domain object.
         contributor_id_to_add: str. ID of the contributor to be added to the
             collection summary.
 
@@ -1092,6 +1092,9 @@ def save_new_collection_from_yaml(committer_id, yaml_content, collection_id):
         committer_id: str. ID of the committer.
         yaml_content: str. The yaml content string specifying a collection.
         collection_id: str. ID of the saved collection.
+
+    Returns:
+        Collection. The domain object.
     """
     collection = collection_domain.Collection.from_yaml(
         collection_id, yaml_content)

diff --git a/core/domain/config_domain.py b/core/domain/config_domain.py
@@ -208,6 +208,7 @@ def init_config_property(cls, name, instance):
 
         Args:
             name: str. The name of the configuration property.
+            instance: *. The instance of the configuration property.
         """
         cls._config_registry[name] = instance
 

diff --git a/core/domain/email_manager.py b/core/domain/email_manager.py
@@ -263,7 +263,7 @@ def _send_email_in_transaction():
             recipient_id, recipient_email, sender_id, sender_name_email, intent,
             email_subject, cleaned_html_body, datetime.datetime.utcnow())
 
-    return transaction_services.run_in_transaction(_send_email_in_transaction)
+    transaction_services.run_in_transaction(_send_email_in_transaction)
 
 
 def _send_bulk_mail(
@@ -312,7 +312,7 @@ def _send_bulk_mail_in_transaction(instance_id=None):
             instance_id, recipient_ids, sender_id, sender_name_email, intent,
             email_subject, cleaned_html_body, datetime.datetime.utcnow())
 
-    return transaction_services.run_in_transaction(
+    transaction_services.run_in_transaction(
         _send_bulk_mail_in_transaction, instance_id)
 
 
@@ -931,6 +931,6 @@ def send_test_email_for_bulk_emails(tester_id, email_subject, email_body):
     """
     tester_name = user_services.get_username(tester_id)
     tester_email = user_services.get_email_from_user_id(tester_id)
-    return _send_email(
+    _send_email(
         tester_id, tester_id, feconf.BULK_EMAIL_INTENT_TEST,
         email_subject, email_body, tester_email, sender_name=tester_name)
diff --git a/core/domain/exp_domain.py b/core/domain/exp_domain.py
@@ -573,6 +573,10 @@ def to_html(self, params):
 
         Raises:
             Exception: 'params' is not a dict.
+
+        Returns:
+            str. The HTML string that results after stripping
+                out unrecognized tags and attributes.
         """
         if not isinstance(params, dict):
             raise Exception(
@@ -867,6 +871,7 @@ def validate(self, interaction, exp_param_specs_dict):
             exp_param_specs_dict: dict. A dict of all parameters used in the
                 exploration. Keys are parameter names and values are ParamSpec
                 value objects with an object type property (obj_type).
+            interaction: InteractionInstance. The interaction object.
 
         Raises:
             ValidationError: One or more attributes of the AnswerGroup are
@@ -1472,11 +1477,11 @@ def update_interaction_hints(self, hints_list):
         """Update the list of hints.
 
         Args:
-            hint_list: list(dict). A list of dict; each dict represents a Hint
+            hints_list: list(dict). A list of dict; each dict represents a Hint
                 object.
 
         Raises:
-            Exception: 'hint_list' is not a list.
+            Exception: 'hints_list' is not a list.
         """
         if not isinstance(hints_list, list):
             raise Exception(
@@ -2421,8 +2426,8 @@ def rename_state(self, old_state_name, new_state_name):
         """Renames the given state.
 
         Args:
-            old_state_names: str. The old name of state to rename.
-            new_state_names: str. The new state name.
+            old_state_name: str. The old name of state to rename.
+            new_state_name: str. The new state name.
 
         Raises:
             ValueError: The old state name does not exist or the new state name
@@ -2459,7 +2464,7 @@ def delete_state(self, state_name):
         """Deletes the given state.
 
         Args:
-            state_names: str. The state name to be deleted.
+            state_name: str. The state name to be deleted.
 
         Raises:
             ValueError: The state does not exist or is the initial state of the
-Original file line number
+Diff line change
@@ Expand Up / @@ -208,6 +208,7 @@ def init_config_property(cls, name, instance): @@
             Args:
                 name: str. The name of the configuration property.
+                instance: *. The instance of the configuration property.
             """
             cls._config_registry[name] = instance
@@ Expand Down @@