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

Ci rustfmt v1 #9044

Closed
wants to merge 2 commits into from
Closed

Ci rustfmt v1 #9044

wants to merge 2 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
None : QA

Describe changes:

  • GitHub action to check that rustfmt-ed files are still rustfmt-ed
  • rustfmt some more files which have a small diff

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #9044 (ec92a65) into master (643e674) will increase coverage by 0.05%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9044      +/-   ##
==========================================
+ Coverage   82.24%   82.29%   +0.05%     
==========================================
  Files         969      969              
  Lines      273655   273654       -1     
==========================================
+ Hits       225055   225208     +153     
+ Misses      48600    48446     -154     
Flag Coverage Δ
fuzzcorpus 64.51% <33.33%> (+0.20%) ⬆️
suricata-verify 60.54% <0.00%> (+0.01%) ⬆️
unittests 62.91% <33.33%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files formatted need to remain formatted
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 14755

@catenacyber
Copy link
Contributor Author

@@ -0,0 +1,10 @@
r=0
cat qa/rustfmt.txt | while read i; do
rustfmt $i;
Copy link
Member

Choose a reason for hiding this comment

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

I'd just call cargo fmt then check each file.

@@ -0,0 +1,57 @@
rust/derive/src/applayerframetype.rs
Copy link
Member

Choose a reason for hiding this comment

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

Maybe build this list within the context of the rust/ directory, as in where its valid to run cargo fmt.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Good start. While I'd rather just format master and master-6.0.x in one go at the same time, this is probably more realistic approach to get to whats expected of Rust projects today.

@catenacyber catenacyber mentioned this pull request Jun 26, 2023
@victorjulien
Copy link
Member

replaced by #9072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants