Add a Presubmit to check if a file must be included or imported.

Add a Presubmit to check if a file must be included or imported.

C++ headers must be included.
ObjC files must be imported.

Checks if a file is ObjC:
- It includes @protocol, @interface, @end, @class
- It imports other files.
- It declares a block type

Files that have a guard
can be included or imported.

BUG=677965

[Olivier, 2016-12-23 09:14:13]

olivierrobin@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Olivier, 2016-12-23 09:14:14]

Would something like this be useful?

[Eugene But (OOO till 7-30), 2016-12-23 16:25:09]

lgtm!!!11111oneunoодин

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode780
PRESUBMIT.py:780: """Checks the include/import coherence in file f."""
Could you please document the rules, about @end/@class/@protocol #import. It
worth mentioning that script is neutral on blocks which are part of C as a
language extension.

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode790
PRESUBMIT.py:790: for line in contents:
Could you please add implementation comments. The logic is not trivial here.

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode821
PRESUBMIT.py:821: break
This is a complex body with multiple loops continue and break statements. Is it
possible to extract the content into a separate function?

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode836
PRESUBMIT.py:836: """Checks that C++ files are included, ObjC files imported
nit: s/files/headers

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode843
PRESUBMIT.py:843: for f in
input_api.AffectedFiles(file_filter=FileFilterIncludeImport):
Usage of filter is discouraged by Python Style Guide:
https://google.github.io/styleguide/pyguide.html#Deprecated_Language_Features

Could you please use comprehension lists.

[Olivier, 2016-12-26 14:49:29]

olivierrobin@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Olivier, 2016-12-26 14:49:29]

Thanks.
PTAL

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode780
PRESUBMIT.py:780: """Checks the include/import coherence in file f."""
On 2016/12/23 16:25:08, Eugene But wrote:
> Could you please document the rules, about @end/@class/@protocol #import. It
> worth mentioning that script is neutral on blocks which are part of C as a
> language extension.

Done.

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode790
PRESUBMIT.py:790: for line in contents:
On 2016/12/23 16:25:08, Eugene But wrote:
> Could you please add implementation comments. The logic is not trivial here.

Done.

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode821
PRESUBMIT.py:821: break
On 2016/12/23 16:25:09, Eugene But wrote:
> This is a complex body with multiple loops continue and break statements. Is
it
> possible to extract the content into a separate function?

Done.

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode836
PRESUBMIT.py:836: """Checks that C++ files are included, ObjC files imported
On 2016/12/23 16:25:08, Eugene But wrote:
> nit: s/files/headers

Done.

https://codereview.chromium.org/2598173002/diff/1/PRESUBMIT.py#newcode843
PRESUBMIT.py:843: for f in
input_api.AffectedFiles(file_filter=FileFilterIncludeImport):
On 2016/12/23 16:25:08, Eugene But wrote:
> Usage of filter is discouraged by Python Style Guide:
> https://google.github.io/styleguide/pyguide.html#Deprecated_Language_Features
> 
> Could you please use comprehension lists.

All other tests use this structure and I cannot change it to a satisfactory
comprehension list.
So letting as it is .

[Eugene But (OOO till 7-30), 2016-12-27 17:18:35]

still lgtm

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode780
PRESUBMIT.py:780: """Checks if f is a C++ or an Objective C header.
nit: s/Objective C/Objective-C 

Same comment for other places.

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode847
PRESUBMIT.py:847: for line in contents:
This should give you |line_num| for free:
for line_num, line in enumerate(contents):

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode870
PRESUBMIT.py:870: input_api.ReadFile(included_absolute_path))
Do you want to create a variable for |included_file| to improve the formatting?

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode890
PRESUBMIT.py:890: warnings = []
Do you need this variable here?

[Olivier, 2016-12-28 14:40:05]

Description was changed from

==========
Add a Presubmit to check if a file must be included or imported.

C++ headers must be included.
ObjC files must be imported.

Checks if a file is ObjC:
- It includes @protocol, @interface, @end
- It imports other files.

Files that have a guard
can be included or imported.
==========

to

==========
Add a Presubmit to check if a file must be included or imported.

C++ headers must be included.
ObjC files must be imported.

Checks if a file is ObjC:
- It includes @protocol, @interface, @end, @class
- It imports other files.
- It declares a block type

Files that have a guard
can be included or imported.
==========

[Olivier, 2016-12-28 14:41:11]

olivierrobin@chromium.org changed reviewers:
+ dpranke@chromium.org

[Olivier, 2016-12-28 14:41:12]

+dpranke as owner.

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode780
PRESUBMIT.py:780: """Checks if f is a C++ or an Objective C header.
On 2016/12/27 at 17:18:35, Eugene But wrote:
> nit: s/Objective C/Objective-C 
> 
> Same comment for other places.

Done

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode847
PRESUBMIT.py:847: for line in contents:
On 2016/12/27 at 17:18:34, Eugene But wrote:
> This should give you |line_num| for free:
> for line_num, line in enumerate(contents):

Done

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode870
PRESUBMIT.py:870: input_api.ReadFile(included_absolute_path))
On 2016/12/27 at 17:18:35, Eugene But wrote:
> Do you want to create a variable for |included_file| to improve the
formatting?

Done

https://codereview.chromium.org/2598173002/diff/20001/PRESUBMIT.py#newcode890
PRESUBMIT.py:890: warnings = []
On 2016/12/27 at 17:18:34, Eugene But wrote:
> Do you need this variable here?

Done

[Olivier, 2017-01-03 15:20:49]

Description was changed from

==========
Add a Presubmit to check if a file must be included or imported.

C++ headers must be included.
ObjC files must be imported.

Checks if a file is ObjC:
- It includes @protocol, @interface, @end, @class
- It imports other files.
- It declares a block type

Files that have a guard
can be included or imported.
==========

to

==========
Add a Presubmit to check if a file must be included or imported.

C++ headers must be included.
ObjC files must be imported.

Checks if a file is ObjC:
- It includes @protocol, @interface, @end, @class
- It imports other files.
- It declares a block type

Files that have a guard
can be included or imported.

BUG=677965
==========

[Nico, 2017-01-03 17:15:31]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-01-03 17:15:32]

How much does this add to presubmit runtime?

Is this a big enough problem in practice that we need a 130+ line presubmit for
this that attempts to do some C parsing in python?

[Dirk Pranke, 2017-01-04 02:37:44]

I share thakis's concerns here. Is this check fast? Can we limit it to files in
a few directories if need be?

[Olivier, 2017-01-04 08:16:47]

On 2017/01/04 02:37:44, Dirk Pranke wrote:
> I share thakis's concerns here. Is this check fast? Can we limit it to files
in
> a few directories if need be?

We can limit to path containing an ios or mac directory and files containing
_ios\W or _mac\W
WDYT?

[Dirk Pranke, 2017-01-04 20:30:08]

On 2017/01/04 08:16:47, Olivier Robin wrote:
> On 2017/01/04 02:37:44, Dirk Pranke wrote:
> > I share thakis's concerns here. Is this check fast? Can we limit it to files
> in
> > a few directories if need be?
> 
> We can limit to path containing an ios or mac directory and files containing
> _ios\W or _mac\W
> WDYT?

I think that would probably be good enough. thakis@, thoughts?

[Olivier, 2017-01-19 16:15:06]

Hi
I added a test on file name
Tell me if this looks enough for performance

Thanks

[Dirk Pranke, 2017-01-20 02:31:35]

this basically looks okay, just a couple of minor comments and I expect the next
rev will be fine.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode800
PRESUBMIT.py:800: An Objective-C header is a header with a line starting with
Nit: blank line between 799 and 800.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode814
PRESUBMIT.py:814: included or imported.
I'm not sure what this paragraph is trying to say, but I suspect it's also not
really necessary and this level of detail should just be in the code.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode817
PRESUBMIT.py:817: the first Objective-C guard, the file is returned as
Objective-C.
same comment.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode837
PRESUBMIT.py:837:
r'\s*(@class|@interface|@end|@implementation|@protocol|#import).*')
This will recompile the regexps for every affected file. Can you move these to
file-level globals (effectively constants)?

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode842
PRESUBMIT.py:842: previous_was_noobjc_define = True
shouldn't this get reset to false at some point during the loop, if the next if
doesn't fire?

[Olivier, 2017-01-27 13:27:19]

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode800
PRESUBMIT.py:800: An Objective-C header is a header with a line starting with
On 2017/01/20 02:31:34, Dirk Pranke wrote:
> Nit: blank line between 799 and 800.

Done.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode814
PRESUBMIT.py:814: included or imported.
On 2017/01/20 02:31:34, Dirk Pranke wrote:
> I'm not sure what this paragraph is trying to say, but I suspect it's also not
> really necessary and this level of detail should just be in the code.

Done.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode817
PRESUBMIT.py:817: the first Objective-C guard, the file is returned as
Objective-C.
On 2017/01/20 02:31:34, Dirk Pranke wrote:
> same comment.

Done.

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode837
PRESUBMIT.py:837:
r'\s*(@class|@interface|@end|@implementation|@protocol|#import).*')
On 2017/01/20 02:31:34, Dirk Pranke wrote:
> This will recompile the regexps for every affected file. Can you move these to
> file-level globals (effectively constants)?

I tried various things, but could not find a good way to do it.
input_api is not available at global level.
Should Import re and use it to compile the regexps?

https://codereview.chromium.org/2598173002/diff/60001/PRESUBMIT.py#newcode842
PRESUBMIT.py:842: previous_was_noobjc_define = True
On 2017/01/20 02:31:34, Dirk Pranke wrote:
> shouldn't this get reset to false at some point during the loop, if the next
if
> doesn't fire?

mmm yes, Done

[Dirk Pranke, 2017-01-29 00:05:11]

Where is this limiting things to particular files or directories?

[Olivier, 2017-01-30 09:09:31]

Hi

This is the first test of _CheckHeaderIsObjectiveC
820   if not file_path_ios_mac_pattern.match(included_file):
821     return (False, False)

Thanks

[Dirk Pranke, 2017-01-30 19:03:11]

On 2017/01/30 09:09:31, Olivier Robin wrote:
> Hi
> 
> This is the first test of _CheckHeaderIsObjectiveC
> 820   if not file_path_ios_mac_pattern.match(included_file):
> 821     return (False, False)
> 
> Thanks

Ah, right, thanks.

@thakis, does this look good to you?

[Olivier, 2017-02-14 07:59:00]

On 2017/01/30 19:03:11, Dirk Pranke wrote:
> On 2017/01/30 09:09:31, Olivier Robin wrote:
> > Hi
> > 
> > This is the first test of _CheckHeaderIsObjectiveC
> > 820   if not file_path_ios_mac_pattern.match(included_file):
> > 821     return (False, False)
> > 
> > Thanks
> 
> Ah, right, thanks.
> 
> @thakis, does this look good to you?

Ping

[sdefresne, 2017-05-09 13:48:59]

sdefresne@chromium.org changed reviewers:
- sdefresne@chromium.org