[Scripts - 3] Added types for pylint and docstring_checker #16548

aasiffaizal · 2022-11-15T02:29:56Z

Overview

This PR fixes or fixes part of #[fill_in_number_here].
This PR does the following: This PR covers type annotation for the files scripts/docstrings_checker.py, scripts/linters/pylint_extensions.py .

Essential 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 linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

Make sure to follow the instructions for making a code change.
If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
Never force push. If you do, your PR will be closed.
Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

oppiabot · 2022-11-15T02:29:57Z

Hi @aasiffaizal, can you complete the following:

The body of this PR is missing the required description, please update the body with a description of what this PR does.
The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
Thanks!

oppiabot · 2022-11-15T02:30:00Z

Hi @aasiffaizal please assign the required reviewer(s) for this PR. Thanks!

aasiffaizal · 2022-11-15T14:39:19Z

scripts/docstrings_checker.py

+# Thus to avoid MyPy's error
+# (Class cannot subclass 'BaseChecker' (has type 'Any')),
+# we added an ignore here.
+class GoogleDocstring(_check_docs_utils.GoogleDocstring):  # type: ignore[misc]


Pylint is partially typed and has the use of many Any, and hence it throws an error when inheriting any classes from them. Since stubs weren't available and specific stubbing of what we use wasn't also working since mypy doesn't support partial stubbing and can be found over here

Agree but please create a different issue and mention it here, because issue #15613 was filed for missing stubs of the apache beam library. ( or if you want to use the same issue, then please update the content of #15613 ).

Just to close the conversation!

A new issue is created refer: #16548 (comment)

aasiffaizal · 2022-11-15T15:35:57Z

@vojtechjelinek @sahiljoster32 @U8NWXD Could you, please take a look?

scripts/linters/pylint_extensions.py

vojtechjelinek

Thanks! Took a pass.

scripts/linters/pylint_extensions.py

aasiffaizal · 2022-11-17T03:51:40Z

@vojtechjelinek @U8NWXD I've addressed all the comments. Can you PTAL?

sahiljoster32

PR looks good just some minor nits, Thanks @aasiffaizal!

sahiljoster32 · 2022-11-17T05:04:44Z

scripts/linters/pylint_extensions.py

        """Checks whether the object class in a module has been documented
        or not. If the object class is not documented then adds an error
        message.

        Args:
-            tokens: Token. Object to access all tokens of a module.
-            node: astroid.scoped_nodes.Module. Node to access module content.
+            tokens: List[Token]. Object to access all tokens of a module.


Suggested change

tokens: List[Token]. Object to access all tokens of a module.

tokens: List[TokenInfo]. Object to access all tokens of a module.

sahiljoster32 · 2022-11-17T05:05:24Z

scripts/linters/pylint_extensions.py

        """Checks whether the Any type in a module has been documented
        or not. If the Any type is not documented then adds an error
        message.

        Args:
-            tokens: Token. Object to access all tokens of a module.
-            node: astroid.scoped_nodes.Module. Node to access module content.
+            tokens: List[Token]. Object to access all tokens of a module.


sahiljoster32 · 2022-11-17T05:09:20Z

scripts/linters/pylint_extensions.py

@@ -2113,7 +2284,13 @@ def _process_module_tokens(self, tokens, node):
                'redundant-type-comment', line=comment_line_number, node=node)


-class SingleSpaceAfterKeyWordChecker(checkers.BaseChecker):
+# TODO(#15613): Here we use MyPy ignore because of the incomplete typing of


I'm a bit confused why #15613 issue used here, I think #15613 is filed for Apache beam stub. I would suggest to create a new issue for this pylint library.

Yeah my bad. I have created a new issue #16567

sahiljoster32 · 2022-11-17T05:09:50Z

scripts/linters/pylint_extensions.py

@@ -2027,7 +2194,9 @@ def visit_module(self, node):
        tokens = pylint_utils.tokenize_module(node)
        self._process_module_tokens(tokens, node)

-    def _process_module_tokens(self, tokens, node):
+    def _process_module_tokens(
+        self, tokens: List[tokenize.TokenInfo], node: astroid.Module


Please update the docstring as well.

sahiljoster32 · 2022-11-17T05:11:32Z

scripts/linters/pylint_extensions.py

@@ -1486,7 +1603,7 @@ class FunctionArgsOrderChecker(checkers.BaseChecker):
            '\'cls\' should come first'),
    }

-    def visit_functiondef(self, node):
+    def visit_functiondef(self, node: astroid.nodes.FunctionDef) -> None:


please update the docstring with FunctionDef

sahiljoster32 · 2022-11-17T05:12:15Z

scripts/linters/pylint_extensions.py

@@ -1348,7 +1437,9 @@ def check_single_constructor_params(self, class_doc, init_doc, class_node):
                args=(class_node.name,),
                node=class_node)

-    def _handle_no_raise_doc(self, excs, node):
+    def _handle_no_raise_doc(
+        self, excs: Set[str], node: astroid.nodes.FunctionDef


update docstring, please

sahiljoster32 · 2022-11-17T05:12:42Z

scripts/linters/pylint_extensions.py

+        self,
+        class_doc: _check_docs_utils.Docstring,
+        init_doc: _check_docs_utils.Docstring,
+        class_node: astroid.nodes.ClassDef


Please update docstring!

sahiljoster32 · 2022-11-17T05:13:38Z

scripts/linters/pylint_extensions.py

@@ -863,7 +918,9 @@ def check_docstring_structure(self, node):
                      any(word in docstring[-2] for word in EXCLUDED_PHRASES)):
                    self.add_message('no-period-used', node=node)

-    def check_docstring_section_indentation(self, node):
+    def check_docstring_section_indentation(
+        self, node: astroid.nodes.FunctionDef


Please update docstring! Function -> FunctionDef

sahiljoster32 · 2022-11-17T05:14:28Z

scripts/linters/pylint_extensions.py

-    def check_functiondef_returns(self, node, node_doc):
+    def check_functiondef_returns(
+        self,
+        node: astroid.nodes.FunctionDef,


Update docstring Function -> FunctionDef

sahiljoster32 · 2022-11-17T05:17:07Z

scripts/docstrings_checker.py

+# Thus to avoid MyPy's error
+# (Class cannot subclass 'BaseChecker' (has type 'Any')),
+# we added an ignore here.
+class GoogleDocstring(_check_docs_utils.GoogleDocstring):  # type: ignore[misc]


Agree but please create a different issue and mention it here, because issue #15613 was filed for missing stubs of the apache beam library. ( or if you want to use the same issue, then please update the content of #15613 ).

oppiabot · 2022-11-17T05:21:23Z

Unassigning @sahiljoster32 since the review is done.

oppiabot · 2022-11-17T05:21:26Z

Hi @aasiffaizal, it looks like some changes were requested on this pull request by @sahiljoster32. PTAL. Thanks!

vojtechjelinek

LGTM!

oppiabot · 2022-11-17T13:12:50Z

Unassigning @vojtechjelinek since they have already approved the PR.

…mber

aasiffaizal · 2022-11-17T17:59:25Z

@sahiljoster32 Addressed all the comments. Could you PTAL?

sahiljoster32

LGTM!, Just one nit!!

scripts/linters/pylint_extensions.py

oppiabot · 2022-11-18T04:29:38Z

Unassigning @sahiljoster32 since they have already approved the PR.

oppiabot · 2022-11-19T00:35:28Z

Hi @aasiffaizal. 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!

oppiabot · 2022-11-19T01:44:04Z

Hi @aasiffaizal, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

sahiljoster32 · 2022-11-19T07:08:11Z

Since all the comments are addressed and all the reviewers approved the PR. I'm going to merge this, Thanks @aasiffaizal!

Added types for pylint and docstring_checker

ddb0588

aasiffaizal added the PR CHANGELOG: Typing -- @vojtechjelinek label Nov 15, 2022

oppiabot bot assigned aasiffaizal Nov 15, 2022

aasiffaizal commented Nov 15, 2022

View reviewed changes

aasiffaizal marked this pull request as ready for review November 15, 2022 15:33

aasiffaizal requested a review from a team November 15, 2022 15:33

aasiffaizal requested a review from a team as a code owner November 15, 2022 15:33

aasiffaizal requested review from sahiljoster32, U8NWXD and vojtechjelinek November 15, 2022 15:33

aasiffaizal assigned vojtechjelinek, sahiljoster32 and U8NWXD Nov 15, 2022

aasiffaizal removed their assignment Nov 15, 2022

U8NWXD requested changes Nov 16, 2022

View reviewed changes

scripts/linters/pylint_extensions.py Outdated Show resolved Hide resolved

U8NWXD assigned aasiffaizal and unassigned U8NWXD Nov 16, 2022

vojtechjelinek suggested changes Nov 16, 2022

View reviewed changes

scripts/linters/pylint_extensions.py Outdated Show resolved Hide resolved

vojtechjelinek removed their assignment Nov 16, 2022

aasiffaizal added 2 commits November 16, 2022 10:45

Merge remote-tracking branch 'upstream/develop' into scripts-typing-3

d3ded9d

Addressed comments

75bc976

aasiffaizal requested review from vojtechjelinek and U8NWXD November 17, 2022 03:50

aasiffaizal assigned vojtechjelinek and U8NWXD and unassigned aasiffaizal Nov 17, 2022

sahiljoster32 suggested changes Nov 17, 2022

View reviewed changes

oppiabot bot unassigned sahiljoster32 Nov 17, 2022

oppiabot bot assigned aasiffaizal Nov 17, 2022

vojtechjelinek approved these changes Nov 17, 2022

View reviewed changes

oppiabot bot unassigned vojtechjelinek Nov 17, 2022

aasiffaizal added 2 commits November 17, 2022 12:56

Merge remote-tracking branch 'upstream/develop' into scripts-typing-3

d07b225

Addressed comments: Udpdated the docstrings and changed TODO issue nu…

fa37fec

…mber

aasiffaizal requested a review from sahiljoster32 November 17, 2022 17:59

aasiffaizal assigned sahiljoster32 and unassigned aasiffaizal Nov 17, 2022

sahiljoster32 approved these changes Nov 18, 2022

View reviewed changes

scripts/linters/pylint_extensions.py Outdated Show resolved Hide resolved

oppiabot bot unassigned sahiljoster32 Nov 18, 2022

Fixed docstring

2ace3dc

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

oppiabot bot assigned aasiffaizal Nov 19, 2022

U8NWXD approved these changes Nov 19, 2022

View reviewed changes

U8NWXD removed their assignment Nov 19, 2022

oppiabot bot added the PR: LGTM label Nov 19, 2022

Merge remote-tracking branch 'upstream/develop' into scripts-typing-3

5583c61

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

sahiljoster32 merged commit ff82e55 into oppia:develop Nov 19, 2022

aasiffaizal deleted the scripts-typing-3 branch November 19, 2022 13:03

	tokens: List[Token]. Object to access all tokens of a module.
	tokens: List[TokenInfo]. Object to access all tokens of a module.

[Scripts - 3] Added types for pylint and docstring_checker #16548

[Scripts - 3] Added types for pylint and docstring_checker #16548

Conversation

aasiffaizal commented Nov 15, 2022 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Nov 15, 2022

oppiabot bot commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aasiffaizal commented Nov 15, 2022

vojtechjelinek left a comment

Choose a reason for hiding this comment

aasiffaizal commented Nov 17, 2022

sahiljoster32 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

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

oppiabot bot commented Nov 17, 2022

oppiabot bot commented Nov 17, 2022

vojtechjelinek left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 17, 2022

aasiffaizal commented Nov 17, 2022

sahiljoster32 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 18, 2022

oppiabot bot commented Nov 19, 2022

oppiabot bot commented Nov 19, 2022

sahiljoster32 commented Nov 19, 2022

aasiffaizal commented Nov 15, 2022 •

edited

Loading