Fix part of issue #8423 Lint concatenation checker #19908

Helper2020 · 2024-03-08T22:27:27Z

Overview

This PR fixes or fixes part of Implement new linter checks #8423].
This PR adds Add a lint check to prevent the usage of string concatenation and promote string interpolation.

Essential Checklist

The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have followed the instructions for making a code change.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

oppiabot · 2024-03-08T22:27:30Z

Hi @Helper2020, can you complete the following:

The body of this PR is missing the overview section, please update it to include the overview.
The body of this PR is missing the proof that changes are correct section, please update it to include the section.
Thanks!

oppiabot · 2024-03-08T22:27:40Z

Assigning @vojtechjelinek for the first pass review of this PR. Thanks!

Helper2020 · 2024-03-08T22:28:34Z

@seanlip PTAL, @U8NWXD PTAL

oppiabot · 2024-03-08T22:31:38Z

Unassigning @Helper2020 since a re-review was requested. @Helper2020, please make sure you have addressed all review comments. Thanks!

seanlip

Thanks @Helper2020! Could you please fix the lint checks that are failing on CI? Thanks.

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2024-03-10T11:51:34Z

scripts/linters/pylint_extensions.py

+            if (isinstance(left_inferred.value, str) and
+                isinstance(right_inferred.value, str)):
+                self.add_message('prefer-string-interpolation', node=node)


Suggested change

if (isinstance(left_inferred.value, str) and

isinstance(right_inferred.value, str)):

self.add_message('prefer-string-interpolation', node=node)

if (

isinstance(left_inferred.value, str) and

isinstance(right_inferred.value, str)

):

self.add_message('prefer-string-interpolation', node=node)

scripts/linters/pylint_extensions_test.py

oppiabot · 2024-03-10T11:55:18Z

Unassigning @vojtechjelinek since the review is done.

U8NWXD · 2024-03-10T18:58:59Z

@Helper2020 please correct the issues flagged by Oppiabot in #19908 (comment):

Hi @Helper2020, can you complete the following:

The body of this PR is missing the overview section, please update it to include the overview.

The body of this PR is missing the proof that changes are correct section, please update it to include the section.
Thanks!

I'm de-assigning myself for now until these issues and CI checks are fixed

Helper2020 · 2024-03-14T21:59:04Z

@vojtechjelinek I'm not sure how I can handle errors from .infer() without try-except blocks. Any suggestions?

vojtechjelinek · 2024-03-15T10:34:32Z

@vojtechjelinek I'm not sure how I can handle errors from .infer() without try-except blocks. Any suggestions?

Why can't you use try-except?

Helper2020 · 2024-03-17T01:37:14Z

@vojtechjelinek I'm not sure how I can handle errors from .infer() without try-except blocks. Any suggestions?

Why can't you use try-except?

I can, just according to the coding style guide, raising exceptions are rarely used.

seanlip · 2024-05-19T05:19:43Z

@Helper2020 How long more do you need?

…_jobs.py

Helper2020 · 2024-05-20T04:44:35Z

@Helper2020 How long more do you need?
@seanlip
I am pretty much done. Although im not sure about the backend test that failed. I don't think it was caused on my end. Can you rerun?

StephenYu2018 · 2024-05-20T21:43:03Z

@Helper2020 Okay I tried re-running the failed jobs.

Helper2020 · 2024-05-21T02:47:55Z

@StephenYu2018 PTAL, @vojtechjelinek PTAL, @seanlip PTAL

oppiabot · 2024-05-21T02:51:00Z

Unassigning @Helper2020 since a re-review was requested. @Helper2020, please make sure you have addressed all review comments. Thanks!

Helper2020 · 2024-05-22T19:28:36Z

@StephenYu2018 PTAL, @vojtechjelinek PTAL, @seanlip PTAL

StephenYu2018

I have no new changes to request. Please address my remaining reviewer comments, as I've left some new comments under those threads. Once that's done, I'll approve these changes.

seanlip

Thanks @Helper2020. Looks great! I just left a few notes on my codeowner files, PTAL.

seanlip · 2024-05-26T08:28:56Z

core/controllers/contributor_dashboard_test.py

+        score_category: str = ('%s%s%s' % (
+            suggestion_models.SCORE_TYPE_TRANSLATION,
+            suggestion_models.SCORE_CATEGORY_DELIMITER, 'English')
+            )


Deindent by 4 (to match opening scope).

seanlip · 2024-05-26T08:30:33Z

core/storage/suggestion/gae_models_test.py

-        suggestion_models.SCORE_TYPE_TRANSLATION +
-        suggestion_models.SCORE_CATEGORY_DELIMITER + 'English')
+        '%s%s%s' % (
+        suggestion_models.SCORE_TYPE_TRANSLATION,


Indent this line and the next by 4 from the previous line, since the ( introduces a new indentation scope.

seanlip · 2024-05-26T08:31:36Z

scripts/linters/html_linter.py

@@ -410,7 +410,7 @@ def _get_trimmed_error_output(html_lint_output: str) -> str:

        for line in html_output_lines:
            trimmed_error_messages.append(line)
-        return '\n'.join(trimmed_error_messages) + '\n'
+        return '\n%s%s' % (''.join(trimmed_error_messages), '\n')


Why not just put the last \n in the main string literal?

seanlip · 2024-05-26T08:31:45Z

scripts/linters/js_ts_linter.py

@@ -526,7 +526,7 @@ def _get_trimmed_error_output(eslint_output: str) -> str:
            else:
                error_message = line
            trimmed_error_messages.append(error_message)
-        return '\n'.join(trimmed_error_messages) + '\n'
+        return '\n%s%s' % (trimmed_error_messages, '\n')


Ditto, put the last \n in the main string literal

seanlip · 2024-05-26T08:32:32Z

scripts/linters/pylint_extensions.py

@@ -2919,6 +2919,55 @@ def visit_importfrom(self, node: astroid.nodes.ImportFrom) -> None:
                self.add_message('disallowed-text-import', node=node)


+# TODO(#16567): Here we use MyPy ignore because of the incomplete typing of


Strike the first "of"

seanlip · 2024-05-26T08:32:46Z

scripts/linters/pylint_extensions.py

+# (Class cannot subclass 'BaseChecker' (has type 'Any')),
+# we added an ignore here.
+class PreventStringConcatenationChecker(checkers.BaseChecker): # type: ignore[misc]
+    """Checks for string concactenation and encourage string interpolation."""


encourage --> encourages

seanlip · 2024-05-26T08:33:39Z

scripts/linters/pylint_extensions.py

+        Args: node: astroid.BinOp The binary operation node to check.
+        """
+
+        if isinstance(node, astroid.BinOp) and node.op == '+':


Not done? (I don't see the changes in the code.)

seanlip · 2024-05-26T08:35:24Z

scripts/setup_test.py

-                '-linux-x64.tar.gz',
+                'https://nodejs.org/dist/v%s/node-v%s%s' % (
+                    common.NODE_VERSION, common.NODE_VERSION,
+                '-linux-x64.tar.gz'),


Why not just add this to the main string, since it is hardcoded?

Helper2020 added 8 commits March 7, 2024 14:27

Added string concatentation class checker

a4bd2a4

Added string concatenation checker tests

1c9655c

Refactored String concatenation checker class

3b8f7b5

Fixed linting issues

908b6cb

Fixed symolic name conflict

7784f5d

Fixed linting issues

c3f9500

Fixing linting issues in pylint extenstions test file

4c0e4b1

Fixed any type error

aa15b24

Helper2020 requested a review from a team as a code owner March 8, 2024 22:27

Helper2020 requested a review from vojtechjelinek March 8, 2024 22:27

oppiabot bot assigned Helper2020 Mar 8, 2024

oppiabot bot assigned vojtechjelinek Mar 8, 2024

oppiabot bot assigned seanlip and U8NWXD and unassigned Helper2020 Mar 8, 2024

seanlip requested changes Mar 9, 2024

View reviewed changes

seanlip assigned Helper2020 and unassigned seanlip Mar 9, 2024

vojtechjelinek requested changes Mar 10, 2024

View reviewed changes

oppiabot bot unassigned vojtechjelinek Mar 10, 2024

U8NWXD removed their assignment Mar 10, 2024

Helper2020 added 3 commits May 19, 2024 13:44

Removed string concatenation from story_fetchers_test

5f25cc8

Removed string concatenations suggestion_services_test.py

ce36f3c

Removed white space from suggestion_services_test.py

ac42de6

Helper2020 requested a review from a team as a code owner May 19, 2024 18:22

Helper2020 requested a review from U8NWXD May 19, 2024 18:22

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

Helper2020 added 10 commits May 19, 2024 14:36

Updated lint-concatenation message id and next message id value

077798c

Removed string concatenations from suggestion_test.py

1ddeed1

Removed string concatenation suggestion_edit_state_contented_deletion…

776fc89

…_jobs.py

Removed string concatenation from common.py

460a176

Removed string concatenation user_validation_jobs_test.py

1ba52b2

Removed string concatenation skill_mastery_test

a1e7f8a

Removed string concatenation reader_test/py

17fd24a

Removed string concatenations suggestion_edit_state_content_jobs_test.py

4caafe9

Fixed spacing in acl_decorators_test.py

d415476

Rephrased String concantenation checker message

17f8e91

oppiabot bot assigned seanlip, StephenYu2018 and vojtechjelinek and unassigned Helper2020 May 21, 2024

StephenYu2018 reviewed May 25, 2024

View reviewed changes

seanlip requested changes May 26, 2024

View reviewed changes

seanlip assigned Helper2020 and unassigned seanlip May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of issue #8423 Lint concatenation checker #19908

Fix part of issue #8423 Lint concatenation checker #19908

Helper2020 commented Mar 8, 2024 •

edited

oppiabot bot commented Mar 8, 2024

oppiabot bot commented Mar 8, 2024

Helper2020 commented Mar 8, 2024

oppiabot bot commented Mar 8, 2024

seanlip left a comment

vojtechjelinek left a comment

vojtechjelinek Mar 10, 2024

Helper2020 May 20, 2024

oppiabot bot commented Mar 10, 2024

U8NWXD commented Mar 10, 2024

Helper2020 commented Mar 14, 2024

vojtechjelinek commented Mar 15, 2024

Helper2020 commented Mar 17, 2024

seanlip commented May 19, 2024

Helper2020 commented May 20, 2024

StephenYu2018 commented May 20, 2024

Helper2020 commented May 21, 2024

oppiabot bot commented May 21, 2024

Helper2020 commented May 22, 2024

StephenYu2018 left a comment

seanlip left a comment

seanlip May 26, 2024

seanlip May 26, 2024

seanlip May 26, 2024

seanlip May 26, 2024

seanlip May 26, 2024

seanlip May 26, 2024

seanlip May 26, 2024

seanlip May 26, 2024

		@@ -2919,6 +2919,55 @@ def visit_importfrom(self, node: astroid.nodes.ImportFrom) -> None:
		self.add_message('disallowed-text-import', node=node)


		# TODO(#16567): Here we use MyPy ignore because of the incomplete typing of

Fix part of issue #8423 Lint concatenation checker #19908

Are you sure you want to change the base?

Fix part of issue #8423 Lint concatenation checker #19908

Conversation

Helper2020 commented Mar 8, 2024 • edited

Overview

Essential Checklist

oppiabot bot commented Mar 8, 2024

oppiabot bot commented Mar 8, 2024

Helper2020 commented Mar 8, 2024

oppiabot bot commented Mar 8, 2024

seanlip left a comment

Choose a reason for hiding this comment

vojtechjelinek left a 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 Mar 10, 2024

U8NWXD commented Mar 10, 2024

Helper2020 commented Mar 14, 2024

vojtechjelinek commented Mar 15, 2024

Helper2020 commented Mar 17, 2024

seanlip commented May 19, 2024

Helper2020 commented May 20, 2024

StephenYu2018 commented May 20, 2024

Helper2020 commented May 21, 2024

oppiabot bot commented May 21, 2024

Helper2020 commented May 22, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

seanlip 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

Helper2020 commented Mar 8, 2024 •

edited