Fix #5876 : To check if class arguments comes in the function then it should come first. #5919

anubhavsinha98 · 2018-11-28T10:04:46Z

Explanation

Fixes #5876 : To check if class arguments comes in function then it should come first.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

codecov-io · 2018-11-28T10:49:34Z

Codecov Report

Merging #5919 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #5919   +/-   ##
=======================================
  Coverage     45.3%   45.3%           
=======================================
  Files          520     520           
  Lines        30667   30667           
  Branches      4597    4597           
=======================================
  Hits         13893   13893           
  Misses       16774   16774

Impacted Files	Coverage Δ
...xploration_player/ExplorationPlayerStateService.js	`1.53% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05ed54f...747280c. Read the comment docs.

lokijuhy · 2018-11-28T12:53:59Z

scripts/pre_commit_linter.py

+    failed = False
+    for filename in files_to_check:
+        with open(filename, 'r') as f:
+            file_content = f.readlines()


The ast library can make this file reading and function identification much simpler. Here's a code snippet you could consider using to iterate over all of the functions in a file and access their args:

for filename in files_to_check: with open(filename, 'r') as f: ast_file = ast.walk(ast.parse(f.read())) func_defs = [n for n in ast_file if isinstance(n, ast.FunctionDef)] for func in func_defs: print func.name print function_node.args.args

lokijuhy · 2018-11-28T13:01:49Z

As we discussed on gitter, I think this code should include tests. Some example tests you would want to write to make sure this code works are:

does a function with a first argument of self pass the check?
does a function with a argument of self that is in the second position fail the check?
does a function with no self or cls arguments pass the check?

Right now you have one function that both reads the file and performs the check. This is challenging to test, because you can't easily pass your function an example to test with. To make this code easier to test, I recommend separating the logic that reads the file from the logic that performs the argument ordering check into two separate functions. That way, you have a standalone function that performs the check, which takes a string or list as input, and therefore you can easily construct example inputs in your tests.

aks681 · 2018-11-29T19:26:15Z

@anubhavsinha98 Was this PR or #5916 created by accident? As both seem to fix the same issue.

lilithxxx · 2018-12-01T20:24:07Z

@lokijuhy I think many checks on pre_commit_linter can use the AST approach so why not shift all of them to use that? @seanlip Whats your thought on this?

seanlip · 2018-12-01T20:39:25Z

I think that would be a good idea, insofar as the checks relate to the actual semantic structure of the code.

anubhavsinha98 · 2018-12-01T21:13:52Z

@lilithxxx, @seanlip and @lokijuhy, I will convert this logic for the AST approach and soon update the PR.

anubhavsinha98 · 2018-12-01T21:16:08Z

And yes it's a nice idea to convert checks in pre_commit_linter, into AST approach.

oppiabot · 2018-12-14T14:27:26Z

Hi @anubhavsinha98, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

oppiabot · 2018-12-19T20:16:11Z

Hi @anubhavsinha98. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

anubhavsinha98 · 2018-12-20T07:07:48Z

@apb7 , done with the custom checker. PTAL ! Thanks !

lokijuhy · 2018-12-20T14:00:20Z

Looks good to me! Great job @anubhavsinha98

apb7

Hi @anubhavsinha98, took an initial pass. Also, how did you test the checker? Can you please post some logs/screenshots? Thanks!

apb7 · 2018-12-20T14:09:23Z

scripts/pre_commit_linter.py

-        newline_messages + docstring_messages + comment_messages +
-        html_tag_and_attribute_messages + html_linter_messages +
-        linter_messages + pattern_messages + copyright_notice_messages)
+        newline_messages + docstring_messages +


Why this change?

Will remove that.

apb7 · 2018-12-20T14:10:37Z

scripts/pylint_extensions.py

@@ -757,6 +757,33 @@ def process_module(self, node):
                        'backslash-continuation', line=line_num + 1)


+class FunctionArgsOrderChecker(checkers.BaseChecker):
+    """Checker for order of arguments. It checks


I think we can have more characters in this line.

Also, this docstring is wrongly phrased. Change it to:
Custom pylint checker which checks the order of arguments in function definition.

Okay will change it .

scripts/pylint_extensions.py

apb7 · 2018-12-20T14:18:05Z

scripts/pylint_extensions.py

+    msgs = {
+        'C0005': ('Wrong order of arguments '
+                  'self should come first',
+                  'wrong-ord-args-self',


The name should be same everywhere. Therefore, change this to function-args-order.

name can't be same as there are two messages so for each message name should be different so that we can differentiate between them.

This is the name of the checker. There is no checker of the name wrong-order-args-self. Did you try running this checker once?

Let's keep a common name and differentiate on the basis of the message.

Actually wrong-ord-args-self is the message-id, which should be unique.

Yes, it is unique for a particular checker. We can distinguish on the basis of the message in this case.

Actually the message_symbol i.e message_id needs to be unique for every message. Otherwise it throws the InvalidMessageError: Message symbol 'function-args-order' is already defined

apb7 · 2018-12-20T14:18:20Z

scripts/pylint_extensions.py

+                  'self should come first',),
+        'C0006': ('Wrong order of arguments '
+                  'cls should come first',
+                  'wrong-ord-args-cls',


apb7 · 2018-12-20T14:18:46Z

scripts/pylint_extensions.py

+    def visit_functiondef(self, node):
+        args_list = [args.name for args in node.args.args]
+        if 'self' in args_list and args_list[0] != 'self':
+            self.add_message('wrong-ord-args-self', node=node)


apb7 · 2018-12-20T14:18:56Z

scripts/pylint_extensions.py

+        if 'self' in args_list and args_list[0] != 'self':
+            self.add_message('wrong-ord-args-self', node=node)
+        elif 'cls' in args_list and args_list[0] != 'cls':
+            self.add_message('wrong-ord-args-cls', node=node)


apb7 · 2018-12-20T14:20:52Z

scripts/pylint_extensions.py

+    name = 'function-args-order'
+    priority = -1
+    msgs = {
+        'C0005': ('Wrong order of arguments '


Break after ( and change this to Wrong order of arguments in function definition. 'self' should come first.

apb7 · 2018-12-20T14:21:51Z

scripts/pylint_extensions.py

+                  'self should come first',
+                  'wrong-ord-args-self',
+                  'self should come first',),
+        'C0006': ('Wrong order of arguments '


Break after ( and change this to Wrong order of arguments in function definition. 'cls' should come first.

anubhavsinha98 · 2018-12-20T16:13:07Z

Tested the checker by making a test.py file !

apb7

Just a couple of comments!

apb7 · 2018-12-20T16:29:59Z

scripts/pylint_extensions.py

+    priority = -1
+    msgs = {
+        'C0005': (
+            'Wrong order of arguments in function definition '


Fullstop at the end of this line?

Will do it.

Sir is that message_id thing ok?

apb7 · 2018-12-20T16:31:35Z

scripts/pylint_extensions.py

+            'function-args-order-self',
+            '\'self\' should come first',),
+        'C0006': (
+            'Wrong order of arguments in function definition '


anubhavsinha98 · 2018-12-20T17:24:25Z

@apb7 Made the changes, PTAL !

apb7

@anubhavsinha98: ptal!

apb7 · 2018-12-21T08:44:59Z

scripts/pylint_extensions.py

@@ -757,6 +757,44 @@ def process_module(self, node):
                        'backslash-continuation', line=line_num + 1)


+class FunctionArgsOrderChecker(checkers.BaseChecker):
+    """Custom pylint checker which checks the order


Can you not accommodate more characters in this line? I feel this has been cut-off too soon..

Yes, will correct it.

apb7 · 2018-12-21T08:45:16Z

scripts/pylint_extensions.py

+    }
+
+    def visit_functiondef(self, node):
+        """Visits every function definition in the


(Also, for the following lines.)

apb7 · 2018-12-21T08:51:56Z

scripts/pylint_extensions_test.py

+
+class FunctionArgsOrderCheckerTest(unittest.TestCase):
+
+    def test_find_function_def(self):


find --> finds

Will change that .

apb7 · 2018-12-21T08:54:41Z

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):


Hmm..I am not sure about making changes here. Shouldn't we incorporate such cases where self and cls both are present as arguments, that self should come first and then cls?

Actually @seanlip suggested these changes, in the #5844(comment) . As there were only two cases present . Well we can add such case also where self and cls both are present and self should come first and then cls. What you suggest?

Oh, then it's fine here. Thanks for the link to the comment!

apb7 · 2018-12-21T14:18:32Z

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):


Oh, then it's fine here. Thanks for the link to the comment!

apb7 · 2018-12-21T14:19:19Z

scripts/pylint_extensions_test.py

+
+
+class FunctionArgsOrderCheckerTest(unittest.TestCase):
+


Hi @vibhor98, do we need docstrings here too, that is, for this test files? Thanks!

Hi @apb7, yes we need to add docstrings to the methods in test files but only selected ones. Those ending with *Test (like this class name) or test_* don't need docstrings. We're handling this via regex.

Cool, thanks a bunch!

apb7

LGTM, thanks!

apb7 · 2018-12-21T18:00:22Z

@anubhavsinha98, can you please confirm that you're using the same email for GitHub as the one with which you've signed the CLA? There seems to be some mismatch in GitHub names for commits:

apb7

Un-approving till this query is resolved. Thanks!

anubhavsinha98 · 2018-12-21T18:04:00Z

@apb7 , I am using same email in Github and the one through which I have signed the CLA .

anubhavsinha98 · 2018-12-21T18:07:48Z

Actually that is my system name !

apb7

Checked with @anubhavsinha98: He has used the same email for signing the CLA as the one used by GitHub. Thanks!

codician added 9 commits November 27, 2018 04:28

First

f115e45

First

28b6e0e

Second

c429cae

Third

83f6a9c

Fourth

9cac86f

Fixes:oppia#5876

55616b9

Fixes:5876

8b66dda

Fixes:5876

c61184a

Fixes:5876

88e9190

anubhavsinha98 requested review from apb7 and seanlip as code owners November 28, 2018 10:04

nithusha21 requested a review from lilithxxx November 28, 2018 11:24

nithusha21 assigned lilithxxx Nov 28, 2018

lokijuhy reviewed Nov 28, 2018

View reviewed changes

nithusha21 mentioned this pull request Dec 11, 2018

Fix #5876 checking if class args comes in any function then it should come first . #5916

Closed

6 tasks

oppiabot bot added the stale label Dec 14, 2018

codician added 6 commits December 14, 2018 22:39

AST Approach

1deda48

AST Approach

bb6cfbf

AST Approach

eb0f56f

AST Approach

68a4e19

AST Approach

3c9386e

AST Approach

7b1e856

Custom Check for Args Order

ba049ea

anubhavsinha98 requested a review from BenHenning as a code owner December 19, 2018 20:12

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Dec 19, 2018

Merge branch 'develop' into mybranch

ed6a7dd

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Dec 19, 2018

codician added 3 commits December 20, 2018 11:13

Added More Tests and resolved merge conflicts

4ba01ec

Added More Tests and resolved merge conflicts

5e1ef9c

Added More Tests and resolved merge conflicts

ac6943f

apb7 suggested changes Dec 20, 2018

View reviewed changes

Requested changes done

3137b6b

apb7 suggested changes Dec 20, 2018

View reviewed changes

codician added 3 commits December 20, 2018 22:09

Added fullstop

7e863d8

Added fullstop

eb8117d

Added docstring

7c317cf

apb7 suggested changes Dec 21, 2018

View reviewed changes

Changes made

747280c

apb7 reviewed Dec 21, 2018

View reviewed changes

apb7 approved these changes Dec 21, 2018

View reviewed changes

apb7 suggested changes Dec 21, 2018

View reviewed changes

apb7 approved these changes Dec 22, 2018

View reviewed changes

apb7 merged commit cd180b2 into oppia:develop Dec 22, 2018

anubhavsinha98 deleted the mybranch branch December 23, 2018 11:25


		class FunctionArgsOrderCheckerTest(unittest.TestCase):

		def test_find_function_def(self):

Fix #5876 : To check if class arguments comes in the function then it should come first. #5919

Fix #5876 : To check if class arguments comes in the function then it should come first. #5919

Conversation

anubhavsinha98 commented Nov 28, 2018 • edited Loading

Explanation

Checklist

codecov-io commented Nov 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lokijuhy commented Nov 28, 2018

aks681 commented Nov 29, 2018

lilithxxx commented Dec 1, 2018

seanlip commented Dec 1, 2018

anubhavsinha98 commented Dec 1, 2018

anubhavsinha98 commented Dec 1, 2018

oppiabot bot commented Dec 14, 2018

oppiabot bot commented Dec 19, 2018

anubhavsinha98 commented Dec 20, 2018

lokijuhy commented Dec 20, 2018

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavsinha98 Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 20, 2018

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 20, 2018

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

apb7 commented Dec 21, 2018

apb7 left a comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 21, 2018

anubhavsinha98 commented Dec 21, 2018 • edited Loading

apb7 left a comment

Choose a reason for hiding this comment

anubhavsinha98 commented Nov 28, 2018 •

edited

Loading

codecov-io commented Nov 28, 2018 •

edited

Loading

anubhavsinha98 Dec 20, 2018 •

edited

Loading

anubhavsinha98 commented Dec 21, 2018 •

edited

Loading