Infer system includes #110
Conversation
There was a problem hiding this comment.
Thanks! Other than my comments, a test case would be useful to help me verify that this does what it intends to do. Perhaps add a test to https://github.com/myint/cppclean/blob/master/test.bash.
cppclean
Outdated
| @@ -88,6 +88,17 @@ def main(): | |||
| help='add a header include path; ' | |||
| 'specify this multiple times for multiple ' | |||
| 'include paths') | |||
| parser.add_argument('--include-path-system', '-is', '-Is', action='append', | |||
There was a problem hiding this comment.
These short options seem a bit at odds with normal POSIX options, where -is would be equivalent to -i and -s.
Could you either rename it or remove it?
cppclean
Outdated
| @@ -88,6 +88,17 @@ def main(): | |||
| help='add a header include path; ' | |||
| 'specify this multiple times for multiple ' | |||
| 'include paths') | |||
| parser.add_argument('--include-path-system', '-is', '-Is', action='append', | |||
| dest='include_system_paths', default=[], | |||
There was a problem hiding this comment.
Could you add a metavar='path' like in the --include-path option. Otherwise, the --help gets a bit messy:
--include-path path, -i path, -I path
add a header include path; specify this multiple times
for multiple include paths
--include-path-system INCLUDE_SYSTEM_PATHS, -is INCLUDE_SYSTEM_PATHS, -Is INCLUDE_SYSTEM_PATHS
same as --include-path but explicitly designates all
header files found in thesedirectories as "system"
includes
--include-path-non-system INCLUDE_NONSYSTEM_PATHS, -in INCLUDE_NONSYSTEM_PATHS, -In INCLUDE_NONSYSTEM_PATHS
same as --include-path but explicitly designates all
header files found in thesedirectories as "non-system"
includes
|
No problem. I just pushed a change that does the following:
|
test_ast.py
Outdated
| @@ -1060,6 +1061,35 @@ def test_system_include(self): | |||
| self.assertEqual(1, len(nodes), repr(nodes)) | |||
| self.assertEqual(Include('vector', system=True), nodes[0]) | |||
|
|
|||
| def test_system_include(self): | |||
There was a problem hiding this comment.
It looks like test_system_include() shadows the test_system_include() that is defined a few lines above.
cppclean
Outdated
| @@ -88,6 +88,19 @@ def main(): | |||
| help='add a header include path; ' | |||
| 'specify this multiple times for multiple ' | |||
| 'include paths') | |||
| parser.add_argument('--include-path-system', '-Is', action='append', | |||
There was a problem hiding this comment.
Could you also remove the -Is and -In for the same reasoning from my previous post. A single letter option (or nothing at all) would be fine.
|
Sure thing. I used "-S" and "-N" for system/non-system includes so there was a shortcut available. I also renamed the test function. Let me know if you need anything else. |
| @@ -88,6 +88,19 @@ def main(): | |||
| help='add a header include path; ' | |||
| 'specify this multiple times for multiple ' | |||
| 'include paths') | |||
| parser.add_argument('--include-path-system', '-S', action='append', | |||
There was a problem hiding this comment.
Let's just use lower case -s and -n. I'll merge it afterwards. Thanks for sticking with this!
There was a problem hiding this comment.
Actually, I can just do that after merging. Thanks!
There was a problem hiding this comment.
No problem. Thanks for merging.
This resolves issue #109