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

[dev_tools] Add script to check whether header guards are style-compliant. #1828

Conversation

cdleary
Copy link
Collaborator

@cdleary cdleary commented Jan 4, 2025

And fix up the non-style-compliant ones.

Wrote this because I forgot one in #1827 and realized I should just write this utility to get the dividends down the road.

Simply run as a script from OSS repo root:

$ python xls/dev_tools/check_header_guards.py 
All header files are compliant.

Copy link
Collaborator

@meheff meheff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup!

Copy link
Collaborator

@allight allight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some lint issues. Need to figure out how to get these into the oss side.

xls/dev_tools/check_header_guards.py Outdated Show resolved Hide resolved
xls/dev_tools/check_header_guards.py Outdated Show resolved Hide resolved
xls/dev_tools/check_header_guards.py Show resolved Hide resolved
@cdleary cdleary requested a review from allight January 7, 2025 05:07
@@ -42,7 +43,8 @@ def check_header_guard(filepath, expected_guard):
return False, None

def find_h_files(repo_root):
# Find all .h files within the repo root, excluding xls/contrib and third_party
# Find all `.h` files within the repo root, excluding `xls/contrib` and
# `third_party`.
h_files = []
for root, _, files in os.walk(repo_root):
if 'xls/contrib' in root or 'third_party' in root:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is or 'third_party' in root needed? I don't think github has this directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdleary cdleary requested a review from hongted January 8, 2025 05:12
@allight
Copy link
Collaborator

allight commented Jan 9, 2025

Ok so this is in internally? IDK What's going on...

@allight
Copy link
Collaborator

allight commented Jan 9, 2025

Ok so it looks like the changes for this somehow got merged into 085321c

Possibly when I migrated it internally to get it to be rebased something got confused?

@cdleary
Copy link
Collaborator Author

cdleary commented Jan 9, 2025

Yeah looks like it's there in 085321c#diff-3b262e89fe8114f1dc9c6a69cb0c059f3df0c65a1c59b203ebc29537d8671061 so will close this PR out.

@cdleary cdleary closed this Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants