Fix extensions #22
Fix extensions #22
Conversation
tkruse
force-pushed
the
fix-extensions
branch
2 times, most recently
from
07894b2 to
c15fdad
Compare
| @@ -6064,7 +6093,7 @@ def ProcessFileData(filename, file_extension, lines, error, | |||
| CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error) | |||
|
|
|||
| # Check that the .cc file has included its header if it exists. | |||
There was a problem hiding this comment.
change also the comment accordingly
|
Excellent review, I made the changes you suggested as additional commit, please look at it when you find the time |
| # The allowed extensions for file names | ||
| # This is set by --extensions flag. | ||
| _valid_extensions = set(['c', 'cc', 'cpp', 'cxx', 'c++', 'h', 'hpp', 'hxx', | ||
| 'h++']) | ||
| _valid_extensions = _nonheader_extensions.union(_header_extensions) |
There was a problem hiding this comment.
what's troubling me now is that this variable will be overridden by --extensions (or the CPPLINT.cfg extensions=...), breaking the relation between it and the header_extensions and nonheader_extensions variables. this will cause some unexpected behaviors...
I suggest that instead you enforce consistency, by not allowing this variable to be overridden directly, but by replacing it with flags and CPPLINT.cfg options for the separate sets (e.g. --headers and --nonheaders), and making sure to update _valid_extensions accordingly when modifying these.
There was a problem hiding this comment.
I think the confusing and unexpected behaviors already exist in the system as it is, because it uses header extensions throughout the code. So while you are right, this PR does not make matters worse than they are. So this would be for a separate PR, I think
There was a problem hiding this comment.
I agree that the confusion already exists, but I disagree that this does not make matters worse.
By separating the extensions to several lists, and exposing only one of them to user-accessible configuration, making it possible that the lists go out of sync - I think this makes things worse (in some aspects) compared to the current state.
Unless I'm missing something...
I'll also be happy with leaving a TODO to fix this in a followup PR, assuming it's coming up soon :-)
Another option is to "unexpose" the extensions option until this gets settled in a followup PR.
There was a problem hiding this comment.
fair enough. I want to stay backwards compatible, to changing the --extension option is not that great. I added a commit that circumvents the problem by not storing in global variables a mixture of defaults and user-specified files. Instead, at runtime functions evaluate those sets. This is more painful to review I guess, but you asked for it. Also this reminds me that I would like unit tests for all these file filters, but I won't have the time soon.
tkruse
force-pushed
the
fix-extensions
branch
2 times, most recently
from
17ddac3 to
618aba9
Compare
|
now also added more text to the usage section as suggested in #17 |
|
sorry, this does not work yet, I do need those tests |
tkruse
force-pushed
the
fix-extensions
branch
2 times, most recently
from
c4050a9 to
6f958f0
Compare
|
@itamaro I wont have time to write tests this week. I fixed the problem though, and you may review the solution for the variables and the usage string. |
Added the headers and extensions option to the CPPLINT.cfg option file
|
Superceded by cpplint/cpplint#14 |
@itamaro, @mattyclarkson, @massenz
Can someone review these changes?
Basically all this does is to allow different file extensions, similar to #17, but with more tests