-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Ci rustfmt v2 #9072
Ci rustfmt v2 #9072
Conversation
Files formatted need to remain formatted
2de5151
to
d19275a
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9072 +/- ##
==========================================
- Coverage 82.35% 82.28% -0.07%
==========================================
Files 969 969
Lines 273655 273654 -1
==========================================
- Hits 225359 225189 -170
- Misses 48296 48465 +169
Flags with carried forward coverage won't be shown. Click here to find out more. |
Information: QA ran without warnings. Pipeline 14828 |
Information:
Pipeline 14830 |
The |
cargo fmt $i; | ||
# the file used to be formatted and is not anymore | ||
if [ $(git diff -- $i | wc -l) -gt 0 ]; then | ||
git diff; |
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 will print diffs of all files, not just the current file.
Probably meant git diff -- $i
?
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.
But cargo fmt $i
only changed one file, right ?
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.
Hmm, looking at that at https://github.com/rust-lang/rustfmt#running
Seems cargo fmt
does the whole crate/module. It might silently ignore the file?
You'd have to use rustfmt
it seems with files. However, rustfmt
might also check some included files if I got the description correct?! So that might check transiently included files multiple times? Ugh, that would open up "interesting behaviours"... Maybe worth trying it out?
If that is the case, it might be easier to run cargo fmt
once and then git diff
check for the list of files in rustfmt.txt
?
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.
Thanks Roland for the tips, waiting for a decision wether this is the right approach before tuning this
cat qa/rustfmt.txt | while read i; do | ||
cargo fmt $i; | ||
# the file used to be formatted and is not anymore | ||
if [ $(git diff -- $i | wc -l) -gt 0 ]; then |
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.
Nothing wrong with this check, just FYI.
An alternative to check for differences would be git diff --quiet -- $i
which makes git diff
"exit with codes similar to diff
". I.e. returns 1 if there are any differences, 0 otherwise. See git help diff
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.
Thanks, looks nicer.
What is the exact bash syntax for 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.
Different ways to skin that cat.
You might have seen something like:
git diff --quiet -- $i
if [ $? -ne 0 ] ; then
# multiple commands ...
fi
I'd probably would do (if you don't need the exit value of the command):
if ! git diff --quiet -- $i; then
# multiple commands ...
fi
Note the inversion with ! ...
as the then
path is executed if the command returns 0. diff
/git diff --quiet
returns 1 (not 0 basically) if files are different.
Or you could short-circuit things
git diff --quiet -- $i || (
# multiple commands ...
)
Or if you fancy one liners - not my personal preference if more than one command in failure path, but valid as well:
git diff --quiet -- $i || ( git diff -- $i; echo "$i needs formatting"; r=1 )
It's really your personal preference
Should I close this, tracked by https://redmine.openinfosecfoundation.org/issues/3836 because the plan is to rustfmt all code when starting on 8 branch ? |
Probably best, ya. |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3836
Describe changes:
#9044 with review taken into account
@jasonish would you know if there is some Github CI way to have both master and PR branch tested : so that we can check rustfmt status of files between without keeping a list of known rustfmt-ed files ?
I am happy with the one-go approach but I guess the incremental one has more chances of getting merged...