Skip to content

Commit

Permalink
Revert "[chromium/src presubmit] Add an inclusive language check func…
Browse files Browse the repository at this point in the history 
…tion."

This reverts commit aad31b6.

Reason for revert: We don't want this running on CI and blocking CLs just yet.

Original change's description:
> [chromium/src presubmit] Add an inclusive language check function.
>
> This change add a new check to prevent non-inlcusive language (e.g.
> 'blacklist', 'whitelist', 'master', 'slave') from being added to the
> repo.
>
> It includes a legacy allowlist file, which lists paths in the repo that
> should be temporarily exempted from this check while we clean up
> existing instances that have accumulated over the years.
>
> Bug: 1177609
> Change-Id: I0d51189134bc506fbe68eacddecdc88360bc86aa
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2718892
> Reviewed-by: Dirk Pranke <dpranke@google.com>
> Commit-Queue: Sean McCullough <seanmccullough@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#860006}

Bug: 1177609
Change-Id: I27355d772526ebf603292e259ba239b7f4eb26be
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2737142
Auto-Submit: Sean McCullough <seanmccullough@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#860034}
  • Loading branch information
Sean McCullough authored and Chromium LUCI CQ committed Mar 5, 2021
1 parent a632542 commit f5cdfea
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 1,384 deletions.
65 changes: 1 addition & 64 deletions PRESUBMIT.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3157,6 +3157,7 @@ def CheckSetNoParent(input_api, output_api):
if directive.startswith('file://'):
if directive in allowed_owners_files:
found_owners_files.add(glob)

# Check that every set noparent line has a corresponding file:// line
# listed in build/OWNERS.setnoparent. An exception is made for top level
# directories since src/OWNERS shouldn't review them.
Expand Down Expand Up @@ -5337,67 +5338,3 @@ def ModifiedPrefMigration(affected_file):
potential_problems
)]
return []

# string pattern, sequence of strings to show when pattern matches,
# error flag. True if match is a presubmit error, otherwise it's a warning.
_NON_INCLUSIVE_TERMS = (
(
# Note that \b pattern in python re is pretty particular. In this regexp,
# 'class WhiteList ...' will match, but 'class FooWhiteList ...' will not.
# This may require some tweaking to catch these cases without triggering
# a lot of false positives. Leaving it naive and less matchy for now.
r'/\b(?i)((black|white)list|master|slave)\b',
('Please don\'t use blacklist, whitelist, master, or slave in your code',
'and make every effort to use other terms. Using "// nocheck" at the end',
'of the offending line will bypass this PRESUBMIT error but avoid using',
'this whenever possible. Reach out to community@chromium.org if you have',
' questions'),
False
),
)

def CheckInclusiveLanguage(input_api, output_api):
"""Make sure that banned non-inclusive terms are not used."""
warnings = []
errors = []

# Note that this matches exact path prefixes, and does not match
# subdirectories. Only files directly in an exlcluded path will
# match.
def IsExcludedFile(affected_file, excluded_paths):
local_dir = input_api.os_path.dirname(affected_file.LocalPath())
return local_dir in excluded_paths

def CheckForMatch(affected_file, line_num, line, term, message, error):
problems = _GetMessageForMatchingType(input_api, f, line_num, line,
term, message)
if problems:
if error:
errors.extend(problems)
else:
warnings.extend(problems)

excluded_paths = []
f = input_api.ReadFile(input_api.os_path.join(
input_api.change.RepositoryRoot(),
'infra',
'inclusive_language_presubmit_exempt_dirs.txt'))
for line in f.split('\n'):
excluded_paths.append(line.split(' ')[0])

excluded_paths = set(excluded_paths)
for f in input_api.AffectedFiles():
for line_num, line in f.ChangedContents():
for term, message, error in _NON_INCLUSIVE_TERMS:
if IsExcludedFile(f, excluded_paths):
continue
CheckForMatch(f, line_num, line, term, message, error)

result = []
if (warnings):
result.append(output_api.PresubmitPromptWarning(
'Banned non-inclusive language was used.\n' + '\n'.join(warnings)))
if (errors):
result.append(output_api.PresubmitError(
'Banned non-inclusive language was used.\n' + '\n'.join(errors)))
return result
94 changes: 0 additions & 94 deletions PRESUBMIT_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3797,99 +3797,5 @@ def testDeletedMarkerRaisesError(self):
errors[0].message)


class InclusiveLanguageCheckTest(unittest.TestCase):
def testBlockedTerms(self):
input_api = MockInputApi()
input_api.change.RepositoryRoot = lambda: ''

input_api.files = [
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
'some/dir 2 1',
'some/other/dir 2 1',
]),
MockFile('some/ios/file.mm',
['TEST(SomeClassTest, SomeInteraction, blacklist) {',
'}']),
MockFile('some/mac/file.mm',
['TEST(SomeClassTest, SomeInteraction, BlackList) {',
'}']),
MockFile('another/ios_file.mm',
['class SomeTest : public testing::Test blocklist {};']),
MockFile('some/ios/file_egtest.mm',
['- (void)testSomething { V(whitelist); }']),
MockFile('some/ios/file_unittest.mm',
['TEST_F(SomeTest, Whitelist) { V(allowlist); }']),
]

errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
self.assertEqual(1, len(errors))
self.assertTrue('some/ios/file.mm' in errors[0].message)
self.assertTrue('another/ios_file.mm' not in errors[0].message)
self.assertTrue('some/mac/file.mm' in errors[0].message)
self.assertTrue('some/ios/file_egtest.mm' in errors[0].message)
self.assertTrue('some/ios/file_unittest.mm' in errors[0].message)

def testBlockedTermsWithLegacy(self):
input_api = MockInputApi()
input_api.change.RepositoryRoot = lambda: ''

input_api.files = [
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
'some/ios 2 1',
'some/other/dir 2 1',
]),
MockFile('some/ios/file.mm',
['TEST(SomeClassTest, SomeInteraction, blacklist) {',
'}']),
MockFile('some/ios/subdir/file.mm',
['TEST(SomeClassTest, SomeInteraction, blacklist) {',
'}']),
MockFile('some/mac/file.mm',
['TEST(SomeClassTest, SomeInteraction, BlackList) {',
'}']),
MockFile('another/ios_file.mm',
['class SomeTest : public testing::Test blocklist {};']),
MockFile('some/ios/file_egtest.mm',
['- (void)testSomething { V(whitelist); }']),
MockFile('some/ios/file_unittest.mm',
['TEST_F(SomeTest, Whitelist) { V(allowlist); }']),
]

errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
self.assertEqual(1, len(errors))
self.assertTrue('some/ios/file.mm' not in errors[0].message)
self.assertTrue('some/ios/subdir/file.mm' in errors[0].message)
self.assertTrue('another/ios_file.mm' not in errors[0].message)
self.assertTrue('some/mac/file.mm' in errors[0].message)
self.assertTrue('some/ios/file_egtest.mm' not in errors[0].message)
self.assertTrue('some/ios/file_unittest.mm' not in errors[0].message)

def testBlockedTermsWithNocheck(self):
input_api = MockInputApi()
input_api.change.RepositoryRoot = lambda: ''

input_api.files = [
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
'some/dir 2 1',
'some/other/dir 2 1',
]),
MockFile('some/ios/file.mm',
['TEST(SomeClassTest, SomeInteraction, blacklist) { // nocheck',
'}']),
MockFile('some/mac/file.mm',
['TEST(SomeClassTest, SomeInteraction, BlackList) { // nocheck',
'}']),
MockFile('another/ios_file.mm',
['class SomeTest : public testing::Test blocklist {};']),
MockFile('some/ios/file_egtest.mm',
['- (void)testSomething { V(whitelist); } // nocheck']),
MockFile('some/ios/file_unittest.mm',
['TEST_F(SomeTest, Whitelist) { V(allowlist); } // nocheck']),
]

errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
self.assertEqual(0, len(errors))


if __name__ == '__main__':
unittest.main()
Loading

0 comments on commit f5cdfea

Please sign in to comment.