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

@vojtechjelinek PTAL, @seanlip PTAL,

core/controllers/contributor_dashboard_test.py

core/controllers/topic_editor.py

core/domain/email_manager.py

Helper2020 · 2024-06-01T19:46:50Z

core/domain/email_manager_test.py

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


core/domain/exp_services_test.py

core/domain/collection_domain.py

Helper2020 · 2024-06-02T02:16:27Z

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,


vojtechjelinek

Please make sure to actually fix the things I mentioned in my previous comments in ALL files you modify. Get rid of .format(, f-strings, nested formatting ('aaaa %s dddd' % ('%s %s' % 'bbbb cccc').

vojtechjelinek · 2024-06-02T20:59:07Z

core/controllers/suggestion_test.py

+ 'backend file system at /exploration/%s/assets/image/ %s' % (
+ exp_id, 'before submitting this translation again.'),


Suggested change

'backend file system at /exploration/%s/assets/image/ %s' % (

exp_id, 'before submitting this translation again.'),

'backend file system at /exploration/%s/assets/image/ %s'

'before submitting this translation again.' % exp_id,

ditto elsewhere

vojtechjelinek · 2024-06-02T21:05:57Z

core/domain/email_manager.py

 'on or before the planned date or adjust the planned publication date.'
 '<br><br>'
 '<ol>%s</ol>'
- ),
+ ).format(constants.CHAPTER_PUBLICATION_NOTICE_PERIOD_IN_DAYS),


Why not use %?

When I used %, the variable constants.Chapter.... when into the %s in between the ol tags.

vojtechjelinek · 2024-06-02T21:06:13Z

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

))

vojtechjelinek · 2024-06-02T21:07:13Z

core/domain/email_manager_test.py

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


Why have nested formatting, can you move '%s%s/story_1' to the string above?

vojtechjelinek · 2024-06-02T21:07:41Z

core/domain/feedback_services_test.py

 entity_type='exploration', entity_id=self.EXP_ID_3,
 original_author_id=self.user_id, subject='Feedback',
 status=feedback_models.STATUS_CHOICES_OPEN, message_count=0,
 has_suggestion=False)
 thread_3.update_timestamps()
 thread_3.put()
 feedback_services.create_message(
- 'exploration.' + self.EXP_ID_3 + '.' + self.THREAD_ID,
+ f'exploration.{self.EXP_ID_3}.{self.THREAD_ID}',


Helper2020

PTAL @vojtechjelinek , hope I didn't miss anything!

Helper2020 · 2024-06-02T21:57:07Z

core/controllers/suggestion_test.py

+ 'backend file system at /exploration/%s/assets/image/ %s' % (
+ exp_id, 'before submitting this translation again.'),


Helper2020 · 2024-06-02T22:15:46Z

core/domain/email_manager.py

 'on or before the planned date or adjust the planned publication date.'
 '<br><br>'
 '<ol>%s</ol>'
- ),
+ ).format(constants.CHAPTER_PUBLICATION_NOTICE_PERIOD_IN_DAYS),


When I used %, the variable constants.Chapter.... when into the %s in between the ol tags.

Helper2020 · 2024-06-02T22:17:44Z

core/domain/email_manager.py

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


Helper2020 · 2024-06-02T23:19:19Z

core/domain/email_manager_test.py

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


Helper2020 · 2024-06-02T23:47:23Z

core/domain/feedback_services_test.py

 entity_type='exploration', entity_id=self.EXP_ID_3,
 original_author_id=self.user_id, subject='Feedback',
 status=feedback_models.STATUS_CHOICES_OPEN, message_count=0,
 has_suggestion=False)
 thread_3.update_timestamps()
 thread_3.put()
 feedback_services.create_message(
- 'exploration.' + self.EXP_ID_3 + '.' + self.THREAD_ID,
+ f'exploration.{self.EXP_ID_3}.{self.THREAD_ID}',


U8NWXD

I've reviewed everything but the extension code, which I'll get to in the next 48 hrs

U8NWXD · 2024-06-03T13:21:55Z

scripts/common.py

- 'logged-in-user/subscribe-to-creator-and-view-all-explorations-' +
-  'by-that-creator',
+ 'logged-in-user/subscribe-to-creator-and-view-all-explorations-'
+ 'by-that-creator',


I think this is actually harder to read without the indentation. When quickly scanning this list, most readers will assume that each line is a list entry, but that's not the case for by-that-creator. I recommend either keeping the indentation or indenting the whole list entry and wrapping in parentheses like this:

'logged-out-user/click-all-links-on-get-started-page', ( 'logged-in-user/subscribe-to-creator-and-view-all-explorations-' 'by-that-creator' ),

U8NWXD · 2024-06-03T13:24:03Z

scripts/linters/html_linter.py

- return '\n'.join(trimmed_error_messages) + '\n'
+ return '\n%s\n' % ''.join(trimmed_error_messages)


This changes behavior. If trimmed_error_messages were the list [1, 2, 3], the original code would produce 1\n2\n3\n, but your new code will produce \n123\n.

U8NWXD · 2024-06-03T13:24:34Z

scripts/linters/js_ts_linter.py

- return '\n'.join(trimmed_error_messages) + '\n'
+ return '\n%s\n' % trimmed_error_messages



ditto: changes behavior

U8NWXD · 2024-06-03T13:30:25Z

scripts/run_frontend_tests.py

- 'http://localhost:9876/base/core/templates/' +
+ 'http://localhost:9876/base/core/templates/'
 'combined-tests.spec.js',


ditto: indent with parentheses to make it clear that these two lines are a single list entry

oppiabot · 2024-06-03T13:40:13Z

Unassigning @U8NWXD since the review is done.

oppiabot · 2024-06-04T19:18:46Z

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!

U8NWXD

Finished reviewing

U8NWXD · 2024-06-04T16:13:52Z

scripts/linters/pylint_extensions.py

+ if any(isinstance(inferred, (astroid.Instance, astroid.Const)) and
+ isinstance(inferred.pytype(), str) and
+ 'datetime.datetime' in inferred.pytype() 
+ for inferred in [left_inferred, right_inferred]):


Can you leave a comment explaining what case this is designed to handle? Also please fix the style:

Suggested change

if any(isinstance(inferred, (astroid.Instance, astroid.Const)) and

isinstance(inferred.pytype(), str) and

'datetime.datetime' in inferred.pytype()

for inferred in [left_inferred, right_inferred]):

if any(

isinstance(inferred, (astroid.Instance, astroid.Const)) and

isinstance(inferred.pytype(), str) and

'datetime.datetime' in inferred.pytype()

for inferred in [left_inferred, right_inferred]

):

The above change fixes 2 things:

When code inside brackets/parentheses needs to be split multiple lines, the whole contents of the brackets/parentheses should be indented in a single block rather than treating the first line specially and only indenting the later lines. This is enforced by a linter for lists, but the same principle applies here.

Follows the same indentation pattern as the next if statement for long if conditions

U8NWXD · 2024-06-04T19:57:50Z

scripts/linters/pylint_extensions_test.py

+ with self.checker_test_object.assertNoMessages():
+ self.checker_test_object.checker.visit_binop(node)
+
+ def test_does_not_encourage_string_interpolation_with_datetime_concatenation(self) -> None: # pylint: disable=line-too-long


This isn't really a concatentation; it's more like addition since the timedelta is being added to the timestamp in the datetime object

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 12 commits June 1, 2024 21:19

Refactored strings

2beb2c5

Refactored strings

7d4ec01

Refactored strings

175d143

Refactore strings

0a782b0

Refactored string

240f2a8

Refactored string

a357f1d

Refactored Strings

f933e23

Refactored string in setup_test.py

1e1950d

Refactored strings in stetup_test.py

b2eef24

Refactored string in start.py

4ca771b

Fixed linting errors

9d0ce76

Fixed skill_domain_test error

055035a

Helper2020 commented Jun 2, 2024

View reviewed changes

vojtechjelinek requested changes Jun 2, 2024

View reviewed changes

Helper2020 added 7 commits June 2, 2024 17:56

Refactored code in suggestion_test.py

7179f93

Refactored string in task_test.py

c0b4042

Refactored String

75edf33

Refactored email_manager_test

ae0b7ce

Refactores trings feedback_services_test.py

7db2fa5

Refactored string story_domain.py

14a70fa

Refactored sring in suggestions_services_test.py

d0f22a2

Helper2020 commented Jun 3, 2024

View reviewed changes

U8NWXD requested changes Jun 3, 2024

View reviewed changes

oppiabot bot unassigned U8NWXD Jun 3, 2024

U8NWXD self-assigned this Jun 3, 2024

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

U8NWXD requested changes Jun 4, 2024

View reviewed changes

U8NWXD removed their assignment Jun 4, 2024

		'backend file system at /exploration/%s/assets/image/ %s' % (
		exp_id, 'before submitting this translation again.'),

		return '\n'.join(trimmed_error_messages) + '\n'
		return '\n%s\n' % ''.join(trimmed_error_messages)

		return '\n'.join(trimmed_error_messages) + '\n'
		return '\n%s\n' % trimmed_error_messages

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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

U8NWXD 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

oppiabot bot commented Jun 3, 2024

oppiabot bot commented Jun 4, 2024

U8NWXD left a 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