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.

Sign up for GitHub

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 #6449: Added check for CODEOWNERS to ensure that the most important rules are at the end. #6560

Merged
merged 16 commits into from
Jun 13, 2019
Merged

Fix #6449: Added check for CODEOWNERS to ensure that the most important rules are at the end. #6560

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7423c00
Added imp rules check
YashJipkate Apr 6, 2019
bea64b0
Removed unnecessary comment
YashJipkate Apr 6, 2019
c2c94e8
Added comments, alphabetize list and improved error message
YashJipkate Apr 19, 2019
35abb55
Merge branch 'develop' into codeowner-imp-rules
YashJipkate May 1, 2019
9d2e3c3
Review addressed
YashJipkate May 1, 2019
2df107a
Improved error messages
YashJipkate May 3, 2019
78d4b16
Merge and break codeowners check function
YashJipkate May 19, 2019
644c6a2
Fix lint
YashJipkate May 20, 2019
026e091
Merge develop
YashJipkate May 29, 2019
65a4c8f
Address review
YashJipkate May 29, 2019
596f823
Address review
YashJipkate Jun 6, 2019
0b5b9b9
Merge branch 'develop' into codeowner-imp-rules
YashJipkate Jun 12, 2019
1c3f7c4
Changed working rule to use set operations
YashJipkate Jun 12, 2019
7e2ad7c
Fix lint
YashJipkate Jun 12, 2019
283997d
Address review
YashJipkate Jun 13, 2019
6679d6c
Fix lint
YashJipkate Jun 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address review
  • Loading branch information
@YashJipkate
YashJipkate committed Jun 6, 2019
commit 596f8230cc6c1952cbbfe2e215fcd24f23327f9f
27 changes: 14 additions & 13 deletions scripts/pre_commit_linter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2235,39 +2235,39 @@ def _check_bad_patterns(self):
return summary_messages

def check_for_important_rules_at_bottom_of_codeowners(
self, imp_patterns, failed):
self, important_patterns):
"""Checks that the most important rules/patterns are at the bottom
of the CODEOWNERS file.

Arguments:
imp_patterns: list(str). List of the important
important_patterns: list(str). List of the important
paths/rules for CODEOWNERS file.
Copy link
Member

Choose a reason for hiding this comment

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

Here you say "paths/rules", but in the docstring you say "rules/patterns", and to make things even more confusing, this arg is called "important_patterns".

Please could you figure out what is actually being referenced, and amend the docstrings/argname/descriptions accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have generalized "patterns".

failed: bool. Current status of failure.

Returns:
bool. Status of failure.
Copy link
Member

Choose a reason for hiding this comment

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

bool. Whether the CODEOWNERS "important rule" check passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"- check fails" would be more appropriate I guess since the bool actually has the status of failure.

"""

failed = False
# The following condition checks for extra rules in the critical
Copy link
Member

Choose a reason for hiding this comment

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

Would it perhaps be cleaner to:

  • Make a set A of important paths from the constant in this file
  • Make a set B of important paths from the CODEOWNERS file
  • Use set difference operations (A-B, B-A) in Python to figure out what's missing in either

I think that would be cleaner and easier to read than printing everything individually and doing lots of for loops.

Copy link
Member

Choose a reason for hiding this comment

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

@seanlip I was too thinking of doing this cleanly using set, but the only issue over here is with repeated patterns, they won't get caught! (If we want to catch repeated pattern we can create check_no_duplicate iteam_in_list before creating a set.)

(Repeated pattern in the ciriticle path patterns.)

Copy link
Member

Choose a reason for hiding this comment

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

Good call! A common pattern for this is to start by doing it with a list, then convert it to set and check if the lengths match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL if it matches with your thoughts @seanlip @DubeySandeep

# files section in the .github/CODEOWNERS file.
if len(imp_patterns) > len(
if len(important_patterns) > len(
CODEOWNER_IMPORTANT_PATHS):
important_path_match_bool_list = ([
imp_path in CODEOWNER_IMPORTANT_PATHS
for imp_path in imp_patterns])
important_path in CODEOWNER_IMPORTANT_PATHS
for important_path in important_patterns])
for index, bool_value in enumerate(
important_path_match_bool_list):
Copy link
Member

Choose a reason for hiding this comment

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

Can we traverse the imp_rules_in_critical_section and do an if check in the loop? (Instead of creating a new list and lopping over that new list) I think having just one loop will make it readable and clear. Let me know if there's an issue with that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think that having a separate variable for this would be more readable similar to the modularization we did for this function as we concluded that splitting up would be better for the end user.

if not bool_value:
print ('%s --> The rule \'%s\' is not an important '
'rule. Please place it before the critical '
'files section.' % (
CODEOWNER_FILEPATH,
imp_patterns[index]))
important_patterns[index]))
failed = True
else:
important_path_match_bool_list = ([
imp_path in imp_patterns
for imp_path in CODEOWNER_IMPORTANT_PATHS])
important_path in important_patterns
for important_path in CODEOWNER_IMPORTANT_PATHS])
# This condition checks that all the values in the boolean list
# are True. This ensures that the critical files of the
# CODEOWNERS file matches with the CODEOWNER_IMPORTANT_PATHS.
Expand Down Expand Up @@ -2303,7 +2303,7 @@ def _check_codeowner_file(self):
# Checks whether every pattern in the CODEOWNERS file matches at
# least one dir/file.
critical_file_section_found = False
imp_rules_in_critical_section = []
important_rules_in_critical_section = []
file_patterns = []
dir_patterns = []
for line_num, line in enumerate(FileCache.readlines(
Expand All @@ -2322,7 +2322,7 @@ def _check_codeowner_file(self):
# This is being populated for the important rules
# check.
if critical_file_section_found:
imp_rules_in_critical_section.append(
important_rules_in_critical_section.append(
line_in_concern)
# Checks if the path is the full path relative to the
# root oppia directory.
Expand Down Expand Up @@ -2381,8 +2381,9 @@ def _check_codeowner_file(self):
print '%s is not covered under CODEOWNERS' % file_path
failed = True

failed = self.check_for_important_rules_at_bottom_of_codeowners(
imp_rules_in_critical_section, failed)
failed = failed or (
self.check_for_important_rules_at_bottom_of_codeowners(
important_rules_in_critical_section))

if failed:
summary_message = '%s CODEOWNERS file check failed' % (
Expand Down