Skip to content
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

Milestone 1.1: Add a check restricting defining type as any #9257

Merged
merged 9 commits into from May 7, 2020
Merged

Milestone 1.1: Add a check restricting defining type as any #9257

merged 9 commits into from May 7, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented May 6, 2020

Overview

1. This PR fixes or fixes part of #[fill_in_number_here].
2. This PR does the following: Add a check restricting defining type as any

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

PR Pointers

  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • 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.

@oppiabot
Copy link

oppiabot bot commented May 6, 2020

Hi, @nishantwrp. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few comments!


files_to_check = self.all_filepaths
patterns_to_match = [r':\ *any', r'^\ *any']
stdout = sys.stdout
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaring this variable is quite useless, just use sys.stdout in the linter_utils.redirect_stdout directly.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

python_utils.PRINT('Starting any type check')
python_utils.PRINT('----------------------------------------')

files_to_check = self.all_filepaths
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaring this variable is quite useless, just use self.all_filepaths in the for file_path in files_to_check directly.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

scripts/linters/excluded_any_type_files.json Show resolved Hide resolved
with python_utils.open_file(EXCLUDE_ANY_TYPE_FILES_PATH, 'r') as f:
EXCLUDED_FILES_ANY_TYPE_CHECK = json.loads(f.read())
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use json.load to open the file directly?

Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also maybe FILES_EXCLUDED_FROM_ANY_TYPE_CHECK.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -46,6 +47,12 @@
# pylint: enable=wrong-import-order
# pylint: enable=wrong-import-position

EXCLUDE_ANY_TYPE_FILES_PATH = os.path.join(
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that FILES_EXCLUDED_FROM_ANY_TYPE_CHECK_PATH would be better.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

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 Show resolved Hide resolved
python_utils.PRINT('----------------------------------------')

files_to_check = self.all_filepaths
patterns_to_match = [r':\ *any', r'^\ *any']
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain these two a bit?

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments. Does that make it clear?

Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but these cases are both quite different one is used on every line the other just on some lines. So I would separate them into two variables.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

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 Show resolved Hide resolved
python_utils.PRINT(
'%s --> ANY type found in this file. Line no.'
' %s' % (
file_path, line_number + 1))
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be moved to the previous line?

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 6, 2020

@vojtechjelinek I have addressed the reviews. PTAL!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Another set of comments. Please don't forget to reply or fix all the previous comments!

FILES_EXCLUDED_FROM_ANY_TYPE_CHECK = json.load(open(
FILES_EXCLUDED_FROM_ANY_TYPE_CHECK_PATH, 'r'))
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the file get closed correctly using this? Shouldn't we still use with python_utils.open_file(EXCLUDE_ANY_TYPE_FILES_PATH, 'r') as f and then pass f into json.load?

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# First pattern is used to match cases like these : any
# Second pattern is used when the character of last line was :
# So, we know that this line begins with a type of variable
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# First pattern is used to match cases like these : any
# Second pattern is used when the character of last line was :
# So, we know that this line begins with a type of variable
# First pattern is used to match cases like these: ': any'.
# Second pattern is used when the last character of
# the previous line was ':' so that we know that this line
# begins with a type.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

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 Show resolved Hide resolved
python_utils.PRINT('----------------------------------------')

files_to_check = self.all_filepaths
patterns_to_match = [r':\ *any', r'^\ *any']
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but these cases are both quite different one is used on every line the other just on some lines. So I would separate them into two variables.

if ((starts_with_type and re.findall(
patterns_to_match[1], line)) or re.findall(
patterns_to_match[0], line)):
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite this if into two cases maybe, so it is more clear to read.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 6, 2020

@vojtechjelinek Done. PTAL!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few last smaller comments.

scripts/linters/js_ts_linter.py Outdated Show resolved Hide resolved
python_utils.PRINT('Starting any type check')
python_utils.PRINT('----------------------------------------')

# pylint: disable=invalid-punctuation-used
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this added?

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

: is a invalid punctuation according to lint checks.

Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hudda Could you look into this, please?

Copy link
Member

@Hudda Hudda May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vojtechjelinek @nishantwrp I ran this code locally and I'm not seeing any error even after removing # pylint: disable=invalid-punctuation-used. @nishantwrp Could you please check one more time.

Copy link
Contributor Author

@nishantwrp nishantwrp May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hudda weird. It's working now. Removed that line.

Copy link
Member

@vojtechjelinek vojtechjelinek May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nishantwrp This was because you didn't have '.' at the end of your comments.

Copy link
Contributor Author

@nishantwrp nishantwrp May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay.

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

with linter_utils.redirect_stdout(sys.stdout):
failed = False
summary_messages = []
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this from here.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

python_utils.PRINT(summary_message)
python_utils.PRINT('')

return summary_messages
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return summary_messages
return [summary_message]

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

summary_message = (
'%s ANY type check passed' % _MESSAGE_TYPE_SUCCESS)

summary_messages.append(summary_message)
Copy link
Member

@vojtechjelinek vojtechjelinek May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.

Copy link
Contributor Author

@nishantwrp nishantwrp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@vojtechjelinek
Copy link
Member

vojtechjelinek commented May 6, 2020

@DubeySandeep @Hudda PTAL

nishantwrp and others added 2 commits May 6, 2020
Co-authored-by: Vojtěch Jelínek <vojtin.j@gmail.com>
@nishantwrp nishantwrp requested a review from vojtechjelinek May 6, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

Hudda
Hudda approved these changes May 7, 2020
Copy link
Member

@Hudda Hudda left a comment

LGTM

@Hudda Hudda removed their assignment May 7, 2020
@vojtechjelinek vojtechjelinek merged commit 119a975 into oppia:develop May 7, 2020
14 checks passed
@nishantwrp nishantwrp deleted the gsoc-any-lint-check branch May 7, 2020
h-ARTS pushed a commit to h-ARTS/oppia that referenced this pull request May 9, 2020
@nishantwrp nishantwrp changed the title Add a check restricting defining type as any Milestone 1.1: Add a check restricting defining type as any May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants