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.

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?

oppiabot · 2024-05-28T18:53:41Z

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

Helper2020

@seanlip PTAL, @StephenYu2018 PTAL, @vojtechjelinek PTAL

vojtechjelinek

Thanks! Took a pass. Please make sure to check if similar stuff that I mention in my comments occurs in other places too.

vojtechjelinek · 2024-05-31T12:20:21Z

core/controllers/contributor_dashboard_test.py

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


Put the last string into the main string. Ditto elsewhere.

vojtechjelinek · 2024-05-31T12:20:59Z

core/controllers/topic_editor.py

@@ -736,7 +736,7 @@ def put(self, topic_id: str) -> None:
            topic_id: str. The ID of the topic.
        """
        assert self.normalized_payload is not None
-        topic_url = feconf.TOPIC_EDITOR_URL_PREFIX + '/' + topic_id
+        topic_url = '%s%s%s' % (feconf.TOPIC_EDITOR_URL_PREFIX, '/', topic_id)


Put the '/' into the main string. Ditto elsewhere.

vojtechjelinek · 2024-05-31T12:21:35Z

core/domain/collection_domain.py

@@ -898,7 +898,7 @@ def _convert_collection_contents_v3_dict_to_v4_dict(
            # this key MyPy throw an error.
            skill_names.update(node['prerequisite_skills']) # type: ignore[misc]
        skill_names_to_ids = {
-            name: _SKILL_ID_PREFIX + str(index)
+            name: '{}{}'.format(_SKILL_ID_PREFIX, index)


Why d you use format and not % here?

Ditto elsewhere

vojtechjelinek · 2024-05-31T12:22:26Z

core/domain/email_manager.py

+    EXPLORATION_ROLE_MANAGER: ('%s%s%s' % (
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_manage'],
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_edit'],
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])
+        ),
+    EXPLORATION_ROLE_EDITOR: ('%s%s' % (
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_edit'],
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])
+        ),
+    EXPLORATION_ROLE_VOICE_ARTIST: ('%s%s' % (
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_voiceover'],
+        _EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])
+        ),


Suggested change

EXPLORATION_ROLE_MANAGER: ('%s%s%s' % (

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_manage'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_edit'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])

),

EXPLORATION_ROLE_EDITOR: ('%s%s' % (

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_edit'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])

),

EXPLORATION_ROLE_VOICE_ARTIST: ('%s%s' % (

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_voiceover'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])

),

EXPLORATION_ROLE_MANAGER: ('%s%s%s' % (

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_manage'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_edit'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])

),

EXPLORATION_ROLE_EDITOR: ('%s%s' % (

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_edit'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])

),

EXPLORATION_ROLE_VOICE_ARTIST: ('%s%s' % (

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_voiceover'],

_EDITOR_ROLE_EMAIL_HTML_RIGHTS['can_play'])

),

vojtechjelinek · 2024-05-31T12:23:06Z

core/domain/email_manager.py

+        story_link = ('%s%s/%s' % (
+            str(feconf.OPPIA_SITE_URL),
+            str(feconf.STORY_EDITOR_URL_PREFIX),
+            overdue_story.id)
+            )


Suggested change

story_link = ('%s%s/%s' % (

str(feconf.OPPIA_SITE_URL),

str(feconf.STORY_EDITOR_URL_PREFIX),

overdue_story.id)

)

story_link = ('%s%s/%s' % (

str(feconf.OPPIA_SITE_URL),

str(feconf.STORY_EDITOR_URL_PREFIX),

overdue_story.id

))

Ditto elsewhere

vojtechjelinek · 2024-05-31T12:24:02Z

core/domain/email_manager_test.py

+            '%s%s/story_1' % (
+                feconf.OPPIA_SITE_URL,
+                feconf.STORY_EDITOR_URL_PREFIX)


Please get rid of this format inside format and just modify the main string.

vojtechjelinek · 2024-05-31T12:24:46Z

core/domain/exp_services_test.py

@@ -6372,14 +6372,14 @@ def test_get_story_id_linked_to_exploration(self) -> None:
        change_list = [
            story_domain.StoryChange({
                'cmd': story_domain.CMD_ADD_STORY_NODE,
-                'node_id': story_domain.NODE_ID_PREFIX + '1',
+                'node_id': f'{story_domain.NODE_ID_PREFIX}1',


Why use f-strings and not %-format?

Ditto elsewhere

oppiabot · 2024-05-31T12:28:39Z

Unassigning @vojtechjelinek since the review is done.

StephenYu2018

LGTM!

oppiabot · 2024-05-31T21:18:08Z

Unassigning @StephenYu2018 since they have already approved the PR.

oppiabot · 2024-05-31T21:18:11Z

Assigning @BenHenning, @U8NWXD for code owner reviews. Thanks!

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

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

Changed test function names in PreventStringConcatenationCheckerTests

ec7fe40

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

Helper2020 added 8 commits May 28, 2024 15:48

deindent 4 spaces to match scope'

a10d7e3

Indented by 4 to match scope

cb60817

Refactor code

301dd2e

Implemented suggestions

e51da8e

Implemented suggested changes

ed8ca0b

Refactor line of code in js_ts_linter.py

c68d081

Removed redudant occurances of the word of

f806aad

Fixed merge conflict

6a12f87

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

Helper2020 added 3 commits May 30, 2024 13:16

Fixed string interpolation error

857dae1

Merge branch 'develop' into lint-concatenation-checker

4077429

Refactored pylint_extensions because of missing coverage

6ec0d18

Helper2020 commented May 31, 2024

View reviewed changes

vojtechjelinek requested changes May 31, 2024

View reviewed changes

oppiabot bot unassigned vojtechjelinek May 31, 2024

StephenYu2018 approved these changes May 31, 2024

View reviewed changes

oppiabot bot unassigned StephenYu2018 May 31, 2024

oppiabot bot assigned BenHenning and U8NWXD May 31, 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

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

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 May 28, 2024

Helper2020 left a comment • edited

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

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 May 31, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

oppiabot bot commented May 31, 2024

oppiabot bot commented May 31, 2024

Helper2020 commented Mar 8, 2024 •

edited

Helper2020 left a comment •

edited