Fix oppia#5876 : To check if class arguments comes in the function th…

…en it should come first. (oppia#5919) * First * First * Second * Third * Fourth * Fixes:oppia#5876 * Fixes:5876 * Fixes:5876 * Fixes:5876 * AST Approach * AST Approach * AST Approach * AST Approach * AST Approach * AST Approach * Solved Two Linting Errors * Solved Two Linting Errors * Solved Two Linting Errors * Solved Two Linting Errors * Solved Two Linting Errors * Solved Two Linting Errors * Solved Two Linting Errors * Travis CI was not building * Custom Check for Args Order * Custom Check for Args Order * Custom Check for Args Order * Custom Check for Args Order * Added More Tests and resolved merge conflicts * Added More Tests and resolved merge conflicts * Added More Tests and resolved merge conflicts * Requested changes done * Added fullstop * Added fullstop * Added docstring * Changes made
quantumferret · Dec 22, 2018 · cd180b2 · cd180b2
1 parent 5481b28
commit cd180b2
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 9 deletions.
diff --git a/extensions/objects/models/objects_test.py b/extensions/objects/models/objects_test.py
@@ -26,25 +26,27 @@
 class ObjectNormalizationUnitTests(test_utils.GenericTestBase):
     """Tests normalization of typed objects."""
 
-    def check_normalization(self, cls, mappings, invalid_items):
+    def check_normalization(self, object_class, mappings, invalid_items):
         """Test that values are normalized correctly.
 
         Args:
-          cls: the class whose normalize() method is to be tested.
+          object_class: the class whose normalize() method is to be tested.
           mappings: a list of 2-element tuples. The first element of
             each item is expected to be normalized to the second.
           invalid_items: a list of values. Each of these is expected to raise
             a TypeError when normalized.
         """
         for item in mappings:
-            assert cls.normalize(item[0]) == item[1], (
+            assert object_class.normalize(item[0]) == item[1], (
                 'Expected %s when normalizing %s as a %s, got %s' %
-                (item[1], item[0], cls.__name__, cls.normalize(item[0]))
+                (
+                    item[1], item[0],
+                    object_class.__name__, object_class.normalize(item[0]))
             )
 
         for item in invalid_items:
             with self.assertRaises(Exception):
-                cls.normalize(item)
+                object_class.normalize(item)
 
     def test_boolean_validation(self):
         """Tests objects of type Boolean."""

diff --git a/extensions/rich_text_components/components_test.py b/extensions/rich_text_components/components_test.py
@@ -26,19 +26,20 @@
 class ComponentValidationUnitTests(test_utils.GenericTestBase):
     """Tests validation of rich text components."""
 
-    def check_validation(self, cls, valid_items, invalid_items):
+    def check_validation(self, rte_component_class, valid_items, invalid_items):
         """Test that values are validated correctly.
 
         Args:
-          cls: the class whose validate() method is to be tested.
+          rte_component_class: the class whose validate() method
+            is to be tested.
           valid_items: a list of values. Each of these items is expected to
             be validated without any Exception.
           invalid_items: a list of values. Each of these is expected to raise
             a TypeError when validated.
         """
         for item in valid_items:
             try:
-                cls.validate(item)
+                rte_component_class.validate(item)
             except Exception as e:
                 self.fail(
                     msg=(
@@ -48,7 +49,7 @@ def check_validation(self, cls, valid_items, invalid_items):
 
         for item in invalid_items:
             with self.assertRaises(Exception):
-                cls.validate(item)
+                rte_component_class.validate(item)
 
     def test_collapsible_validation(self):
         """Tests collapsible component validation."""

diff --git a/scripts/pylint_extensions.py b/scripts/pylint_extensions.py
@@ -757,6 +757,43 @@ def process_module(self, node):
                         'backslash-continuation', line=line_num + 1)
 
 
+class FunctionArgsOrderChecker(checkers.BaseChecker):
+    """Custom pylint checker which checks the order of arguments in function
+    definition.
+    """
+
+    __implements__ = interfaces.IAstroidChecker
+    name = 'function-args-order'
+    priority = -1
+    msgs = {
+        'C0005': (
+            'Wrong order of arguments in function definition '
+            '\'self\' should come first.',
+            'function-args-order-self',
+            '\'self\' should come first',),
+        'C0006': (
+            'Wrong order of arguments in function definition '
+            '\'cls\' should come first.',
+            'function-args-order-cls',
+            '\'cls\' should come first'),
+    }
+
+    def visit_functiondef(self, node):
+        """Visits every function definition in the python file and check the
+        function arguments order. It then adds a message accordingly.
+
+        Args:
+         node: astroid.scoped_nodes.Function. Node for a function or method
+                definition in the AST.
+        """
+
+        args_list = [args.name for args in node.args.args]
+        if 'self' in args_list and args_list[0] != 'self':
+            self.add_message('function-args-order-self', node=node)
+        elif 'cls' in args_list and args_list[0] != 'cls':
+            self.add_message('function-args-order-cls', node=node)
+
+
 def register(linter):
     """Registers the checker with pylint.
 
@@ -768,3 +805,4 @@ def register(linter):
     linter.register_checker(DocstringParameterChecker(linter))
     linter.register_checker(ImportOnlyModulesChecker(linter))
     linter.register_checker(BackslashContinuationChecker(linter))
+    linter.register_checker(FunctionArgsOrderChecker(linter))
diff --git a/scripts/pylint_extensions_test.py b/scripts/pylint_extensions_test.py
@@ -230,3 +230,32 @@ def test_finds_backslash_continuation(self):
             ),
         ):
             temp_file.close()
+
+
+class FunctionArgsOrderCheckerTest(unittest.TestCase):
+
+    def test_finds_function_def(self):
+        checker_test_object = testutils.CheckerTestCase()
+        checker_test_object.CHECKER_CLASS = (
+            pylint_extensions.FunctionArgsOrderChecker)
+        checker_test_object.setup_method()
+        functiondef_node1 = astroid.extract_node("""
+        def test(self,test_var_one, test_var_two): #@
+            result = test_var_one + test_var_two
+            return result
+        """)
+        with checker_test_object.assertNoMessages():
+            checker_test_object.checker.visit_functiondef(functiondef_node1)
+
+        functiondef_node2 = astroid.extract_node("""
+        def test(test_var_one, test_var_two, self): #@
+            result = test_var_one + test_var_two
+            return result
+        """)
+        with checker_test_object.assertAddsMessages(
+            testutils.Message(
+                msg_id='function-args-order-self',
+                node=functiondef_node2
+            ),
+        ):
+            checker_test_object.checker.visit_functiondef(functiondef_node2)