Skip to content
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

fix logic bug in include_checker.py utility #734

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Jul 7, 2022

Hi, I noticed an issue in the include style checker when I was trying to adapt it for use in another project.

As far as I can tell, the current logic results in no source files actually being checked! Fortunately, the script also passes after the proposed change, so the includes were already in good shape :)

I verified the fix here by changing a <...>-style import to a double-quote one in one of the source files and it was detected.

I don't know if this file originates in RAFT or if it is also in other RAPIDS projects as well? If the later, the fix will need to be propagated to other repositories.

The current if statement always evaluates to false, so no files get checked
@grlee77 grlee77 requested a review from a team as a code owner July 7, 2022 22:45
@github-actions github-actions bot added the cpp label Jul 7, 2022
@grlee77
Copy link
Contributor Author

grlee77 commented Jul 7, 2022

the includes were already in good shape :)

Looks like I spoke to soon! The CI found several issues in files under cpp/include. I think I had only run it on cpp/src when testing locally.

It looks like logic to exclude the thirdparty folders needs to be added back in.

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 8, 2022
@grlee77
Copy link
Contributor Author

grlee77 commented Jul 8, 2022

Okay, style check is passing now. (The f variable is only the base filename, so it is the containing folder name that the thirdpartyexclusion check should have been applied to)

@cjnolet
Copy link
Member

cjnolet commented Jul 8, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4f22957 into rapidsai:branch-22.08 Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants