-
Notifications
You must be signed in to change notification settings - Fork 402
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
iox-1930 remove backticks and add commit hook and ci check #1931
iox-1930 remove backticks and add commit hook and ci check #1931
Conversation
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.
LGTM, thanks for contributing this 🎉
grep -rn --include=\*.{h,hpp,inl,c,cpp} "\`" iceoryx_* | ||
} | ||
|
||
BACKTICK_SEARCH_STRING="\`" |
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 script is named check_invaldi_characters
could you please rename this variable into INVALID_CHARACTERS
.
Another thing would be when INVALID_CHARACTERS
becomes an array which would make more sense in the context of this script.
This should be easy, just use space as separator and then iterate of it.
(All of this is purely optional).
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.
@elfenpiff the idea was to just copy&paste that part of the code and add a new invalid character. I was not sure if is is an error if the same variable is defined twice.
If you can confirm that it is not an issue I will change it. But I would not make an array out of it right now since one cannot have a custom error message per character. The complex part is already in a function.
You can of course adjust the script as you like in a follow up PR. This is just an initial minimal viable solution
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 think this is something for FutureBob or FutureElfe
02f8032
to
7c43100
Compare
Codecov Report
@@ Coverage Diff @@
## master #1931 +/- ##
=======================================
Coverage 75.44% 75.44%
=======================================
Files 384 384
Lines 15205 15205
Branches 2148 2148
=======================================
+ Hits 11471 11472 +1
Misses 3062 3062
+ Partials 672 671 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Pre-Review Checklist for the PR Author
Changelog updated in the unreleased section including API breaking changesiox-123-this-is-a-branch
)iox-#123 commit text
)task-list-completed
)iceoryx_hoofs
are added to./clang-tidy-diff-scans.txt
Notes for Reviewer
This PR removes all remaining backticks from the codebase and adds a commit hook and CI check to prevent a reintroduction.
Checklist for the PR Reviewer
iceoryx_hoofs
have been added to./clang-tidy-diff-scans.txt
Post-review Checklist for the PR Author
References