Bug 217972 – Make report-non-inclusive-language ignore autoinstalled directories and certain file types

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

Make report-non-inclusive-language ignore following directories and files:

1) 'autoinstalled': The source code of packages inside autoinstalled directories (e.g.: Scripts/libraries/autoinstalled) are not part of our repository and is not controlled by us. So report-non-inclusive-language shouldn't count them. Currently the count is really skewed because of counting such autoinstalled directories.

2) Files ending with .swp: usually these are files created for files opened in other tabs).

3) Files ending with .log: usually these are not part of source code, and are simply logs. For e.g.: twistd.log while running buildbot instance locally.

[Aakash Jain]

Created attachment 411887 [details]
Patch

[Aakash Jain]

It should also ignore files ending in .pyc

[Aakash Jain]

Created attachment 411892 [details]
Patch

[Beth Dakin]

Comment on attachment 411887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411887&action=review

> Tools/Scripts/report-non-inclusive-language:47
> +IGNORE_FILES_ENDING_WITH = ('.order', '.xcuserstate', '.swp', '.log')

I saw your comment about adding .pyc here, but R+ for now anyway. This is great!

[EWS]

Committed r268752: <https://trac.webkit.org/changeset/268752>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411892 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70496351>

[Darin Adler]

Comment on attachment 411892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411892&action=review

> Tools/Scripts/report-non-inclusive-language:61
> +    if any(directory in prefix for directory in IGNORE_DIRECTORIES):

This is IGNORE_DIRECTORIES or IGNORE_DIRECTORIES_CONTAINING?

It looks like we are using substring matching for directory names, but we want to know if this is exactly a directory name. Ideally this should not match "notautoinstalled".

I’d like a more correct implementation to be future-looking although for now none of those three strings seems like a problem.

I believe that some others at Apple have been using this script, so it’s good if it works well even outside WebKit, which is why it’s good to be careful about such things.

[Aakash Jain]

(In reply to Darin Adler from comment #7)
> It looks like we are using substring matching for directory names, but we want to know if this is exactly a directory name. Ideally this should not match "notautoinstalled".
Good point. Should we do the exact match for the directories. That seems like the right thing to do. Proposed patch would be something like:

-    if any(directory in prefix for directory in IGNORE_DIRECTORIES):
+    if any(directory==prefix for directory in IGNORE_DIRECTORIES):

[Aakash Jain]

(In reply to Aakash Jain from comment #8)
> (In reply to Darin Adler from comment #7)
> > It looks like we are using substring matching for directory names, but we want to know if this is exactly a directory name. Ideally this should not match "notautoinstalled".
> Good point. Should we do the exact match for the directories. That seems
> like the right thing to do. Proposed patch would be something like:
> 
> -    if any(directory in prefix for directory in IGNORE_DIRECTORIES):
> +    if any(directory==prefix for directory in IGNORE_DIRECTORIES):
Ignore my above comment, just noticed that prefix contains the complete path.

[Aakash Jain]

Reopening to attach minor follow-up patch.

[Aakash Jain]

Created attachment 411908 [details]
Patch

[Darin Adler]

Comment on attachment 411908 [details]
Patch

For now, this is OK.

[Darin Adler]

(In reply to Aakash Jain from comment #8)
> (In reply to Darin Adler from comment #7)
> > It looks like we are using substring matching for directory names, but we want to know if this is exactly a directory name. Ideally this should not match "notautoinstalled".
> Good point. Should we do the exact match for the directories. That seems
> like the right thing to do. Proposed patch would be something like:
> 
> -    if any(directory in prefix for directory in IGNORE_DIRECTORIES):
> +    if any(directory==prefix for directory in IGNORE_DIRECTORIES):

Maybe:

    if any(directory.endsWith("/" + prefix) for directory in IGNORE_DIRECTORIES):

Would that work?

[EWS]

Committed r268763: <https://trac.webkit.org/changeset/268763>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411908 [details].

[Aakash Jain]

(In reply to Darin Adler from comment #13)
> Maybe:
> 
>     if any(directory.endsWith("/" + prefix) for directory in
> IGNORE_DIRECTORIES):
> 
> Would that work?
That doesn't seem to work. Uploaded patch in https://bugs.webkit.org/show_bug.cgi?id=217993 which seems to work fine.