-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix part of issue #8423 Lint concatenation checker #19908
base: develop
Are you sure you want to change the base?
Conversation
Hi @Helper2020, can you complete the following:
|
Assigning @vojtechjelinek for the first pass review of this PR. Thanks! |
Unassigning @Helper2020 since a re-review was requested. @Helper2020, please make sure you have addressed all review comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Helper2020! Could you please fix the lint checks that are failing on CI? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Left a few comments.
scripts/linters/pylint_extensions.py
Outdated
if (isinstance(left_inferred.value, str) and | ||
isinstance(right_inferred.value, str)): | ||
self.add_message('prefer-string-interpolation', node=node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Unassigning @vojtechjelinek since the review is done. |
@Helper2020 please correct the issues flagged by Oppiabot in #19908 (comment):
I'm de-assigning myself for now until these issues and CI checks are fixed |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @vojtechjelinek , hope I didn't miss anything!
core/domain/email_manager.py
Outdated
'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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I used %, the variable constants.Chapter.... when into the %s in between the ol tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed everything but the extension code, which I'll get to in the next 48 hrs
scripts/common.py
Outdated
'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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/linters/html_linter.py
Outdated
return '\n'.join(trimmed_error_messages) + '\n' | ||
return '\n%s\n' % ''.join(trimmed_error_messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/linters/js_ts_linter.py
Outdated
return '\n'.join(trimmed_error_messages) + '\n' | ||
return '\n%s\n' % trimmed_error_messages | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: changes behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domne.
scripts/run_frontend_tests.py
Outdated
'http://localhost:9876/base/core/templates/' + | ||
'http://localhost:9876/base/core/templates/' | ||
'combined-tests.spec.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: indent with parentheses to make it clear that these two lines are a single list entry
Unassigning @U8NWXD since the review is done. |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished reviewing
scripts/linters/pylint_extensions.py
Outdated
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]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you leave a comment explaining what case this is designed to handle? Also please fix the style:
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 longif
conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really a concatentation; it's more like addition since the timedelta is being added to the timestamp in the datetime object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Changed function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Took another pass.
core/domain/story_services.py
Outdated
@@ -534,13 +534,16 @@ def validate_prerequisite_skills_in_story_contents( | |||
topic_relevant_skill_ids | |||
).issubset(simulated_skill_ids)): | |||
raise utils.ValidationError( | |||
'The skills with ids ' + | |||
'%s%s%s%s,%s' % ( | |||
'The skills with ids ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, can you move this to the above string?
'The skills with ids %s were specified as prerequisites for Chapter %s '
'but were not taught in any chapter before it.' % (
' '.join(set(topic_relevant_skill_ids) - set(simulated_skill_ids)),
destination_node.title
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
core/domain/suggestion_services.py
Outdated
score_category = ('%s%s%s' % ( | ||
suggestion_models.SCORE_TYPE_CONTENT, | ||
suggestion_models.SCORE_CATEGORY_DELIMITER, exploration.category) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
score_category = ('%s%s%s' % ( | |
suggestion_models.SCORE_TYPE_CONTENT, | |
suggestion_models.SCORE_CATEGORY_DELIMITER, exploration.category) | |
) | |
score_category = ('%s%s%s' % ( | |
suggestion_models.SCORE_TYPE_CONTENT, | |
suggestion_models.SCORE_CATEGORY_DELIMITER, exploration.category | |
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
core/domain/user_services_test.py
Outdated
user_services.get_username('pid_%s' % ('a' * 32)) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user_services.get_username('pid_%s' % ('a' * 32)) | |
) | |
user_services.get_username('pid_%s' % ('a' * 32)) | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto elsewhere
@@ -76,7 +76,7 @@ class TextInput(base.BaseInteraction): | |||
'default_value': 1, | |||
}, { | |||
'name': 'catchMisspellings', | |||
'description': 'Catch Misspellings (Detect if answer is misspelled' + | |||
'description': 'Catch Misspellings (Detect if answer is misspelled' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the string into brackets? The next line is hard to differ from other keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'description': (
'Catch Misspellings (Detect if answer is misspelled '
'and nudge the learner to correct the misspelling)'
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
extensions/interactions/base_test.py
Outdated
@@ -289,8 +289,8 @@ def test_interaction_properties(self) -> None: | |||
'default_value': 1, | |||
}, { | |||
'name': 'catchMisspellings', | |||
'description': 'Catch Misspellings (Detect if answer is' + | |||
' misspelled and nudge the learner to correct the' + | |||
'description': 'Catch Misspellings (Detect if answer is' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @vojtechjelinek, PTAL @U8NWXD
core/domain/story_services.py
Outdated
@@ -534,13 +534,16 @@ def validate_prerequisite_skills_in_story_contents( | |||
topic_relevant_skill_ids | |||
).issubset(simulated_skill_ids)): | |||
raise utils.ValidationError( | |||
'The skills with ids ' + | |||
'%s%s%s%s,%s' % ( | |||
'The skills with ids ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
core/domain/email_manager.py
Outdated
'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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Apparently, to escape the %s is prepend another %?
core/domain/suggestion_services.py
Outdated
score_category = ('%s%s%s' % ( | ||
suggestion_models.SCORE_TYPE_CONTENT, | ||
suggestion_models.SCORE_CATEGORY_DELIMITER, exploration.category) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -76,7 +76,7 @@ class TextInput(base.BaseInteraction): | |||
'default_value': 1, | |||
}, { | |||
'name': 'catchMisspellings', | |||
'description': 'Catch Misspellings (Detect if answer is misspelled' + | |||
'description': 'Catch Misspellings (Detect if answer is misspelled' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
extensions/interactions/base_test.py
Outdated
@@ -289,8 +289,8 @@ def test_interaction_properties(self) -> None: | |||
'default_value': 1, | |||
}, { | |||
'name': 'catchMisspellings', | |||
'description': 'Catch Misspellings (Detect if answer is' + | |||
' misspelled and nudge the learner to correct the' + | |||
'description': 'Catch Misspellings (Detect if answer is' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/linters/pylint_extensions.py
Outdated
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]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Changed function name.
scripts/common.py
Outdated
'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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/linters/html_linter.py
Outdated
return '\n'.join(trimmed_error_messages) + '\n' | ||
return '\n%s\n' % ''.join(trimmed_error_messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/linters/js_ts_linter.py
Outdated
return '\n'.join(trimmed_error_messages) + '\n' | ||
return '\n%s\n' % trimmed_error_messages | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domne.
Overview
Essential Checklist