-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exclude files or directories in cpplint.py #238
Conversation
Signed-off-by: ahcorde <ahcorde@gmail.com>
0f7eaa1
to
d00f4b6
Compare
Signed-off-by: ahcorde <ahcorde@gmail.com>
d00f4b6
to
b9f07a6
Compare
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exclude logic needs to be fixed as pointed out multiple times.
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
ament_cpplint/ament_cpplint/main.py
Outdated
files_to_avoid.append(f) | ||
files_check = list(set(files) ^ set(files_to_avoid)) | ||
|
||
if(len(files_check) != 0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check us only necessary since the previous check from line 123 isn't effective since the exclude logic is applied afterwards. How about moving the logic into get_file_groups()
- which I think would be the most efficient approach since it even avoid crawling / collecting files under an excluded directory. I would expect that would also avoid the need to check all parents
.
Nitpick: no parenthesis around Python conditions. The Pythonic way to write this condition would be: if files_check
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic moved to get_file_groups()
3b9b453
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exclude
logic can already be applied to the dirnames
variable to filter them out early (without the need to recurse into them). That will also simplify the check later which doesn't have to consider parents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to dirnames 3b9b453
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where the code filters dirnames
based on the excludes
?
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
ament_cpplint/ament_cpplint/main.py
Outdated
if 'AMENT_IGNORE' in dirnames + filenames: | ||
dirnames[:] = [] | ||
continue | ||
# ignore folder starting with . or _ | ||
dirnames[:] = [d for d in dirnames if d[0] not in ['.', '_']] | ||
dirnames[:] = [d for d in dirnames if d not in excludes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dirnames
only contains the names of the subdirectories of dirpath
. As such the condition not in excludes
is incorrect again - for the same reasons as the previous comments.
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
ament_cpplint/ament_cpplint/main.py
Outdated
# select files by extension | ||
for filename in sorted(filenames): | ||
_, ext = os.path.splitext(filename) | ||
if ext in ('.%s' % e for e in extensions): | ||
append_file_to_group(groups, | ||
os.path.join(dirpath, filename)) | ||
if filename not in excludes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition still doesn't make sense.
Please write a unit test which tests a variety of cases to validate the implementation against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humm, why not? the excludes argument might contains a filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think an exclude of bar
should match a file foo/bar
. Instead the exclude should state foo/bar
explicitly. Otherwise how do I exclude the file bar
but not the file foo/bar
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I need to include the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to include the test in this PR but as mentioned before writing one will help you to check the logic. Especially the part to consider an example file system structure and excludes and what the filtered result is expected to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dirk-thomas I created a gist with a test: https://gist.github.com/ahcorde/bcc558aedad8560cfa46c4b5de366f32
This is the last commit 6db3ae3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to use the gist. get_file_groups()
seems to always return an empty dict?
Just from reading over the logic I would expect that a file include/test/cache_unittest.cpp
would be incorrectly skipped even though only test/cache_unittest.cpp
is specified in the excludes.
Also I still don't see why is_file_or_directory_to_exclude()
should needs any logic like check_path_parents()
.
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde Any update on this? |
I agree with Dirk on #238 (comment)
If the desire is to match |
The question now is what's the behavior already built into
as matching that would be more important than implementing glob rules. EDIT: The current behavior of |
@ahcorde, what do you think we should do here? Should we close this PR? |
yes, we can close this PR |
This PR is related to this other #234
ament_cmake_cpplint
includes all the files in the package, with this option we can exclude some files or directories.Signed-off-by: ahcorde ahcorde@gmail.com