Skip to content

Add Cleanness CI #2 #619

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

Merged
merged 10 commits into from
Sep 13, 2024
Merged

Add Cleanness CI #2 #619

merged 10 commits into from
Sep 13, 2024

Conversation

okBrian
Copy link
Contributor

@okBrian okBrian commented Sep 9, 2024

Description

Add Cleanness CI 2nd PR
Fails if pr # warning > master # warnings

Fixes #467

Type of change

  • New feature (non-breaking change which adds functionality)

Scope

  • This PR comprises a set of related changes with a common goal

How Has This Been Tested?

[x] Make a commit that includes a new unused variable and check if the CI fails
[x] Make sure once the commit is reverted that it shows -1 and that doesn't fail the CI

Test Configuration:

  • What computers and compilers did you use to test this:
    Github Actions on Ubuntu and GNU compilers

Checklist

  • I ran ./mfc.sh format before committing my code
  • This PR does not introduce any repeated code (it follows the DRY principle)
  • I cannot think of a way to condense this code and reduce any introduced additional line count

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.69%. Comparing base (63c79cb) to head (194adf6).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #619   +/-   ##
=======================================
  Coverage   54.69%   54.69%           
=======================================
  Files          59       59           
  Lines       13662    13662           
  Branches     1698     1698           
=======================================
  Hits         7473     7473           
  Misses       5740     5740           
  Partials      449      449           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sbryngelson
Copy link
Member

sbryngelson commented Sep 9, 2024

I just realized that if we have

Unused Variable Count: 379, Difference: 0

then if Difference: 5 it's going to be rather impossible for the user to find what ones they contributed! Can you use diff or some such to print the additional ones (or removed ones).

Update: Here's the trick

diff <(echo $master_output) <(echo $pr_output)

stolen from: https://stackoverflow.com/questions/13437104/compare-content-of-two-variables-in-bash like a proper script kiddie.

Update: I suppose you probably want something like

diff <(echo $master_output) <(echo $pr_output) | grep -c -F 'Wunused-variable'

or so

@sbryngelson
Copy link
Member

Phoenix CI won't run because the computer is down... but that doesn't matter for this PR.

@okBrian
Copy link
Contributor Author

okBrian commented Sep 9, 2024

So.... I have tried to diff these for the past couple hours. I have been trying a couple different variations on my own branch and none of them have worked for how I think we want them to ie
If Master and PR are different and PR has unique lines then it should output the unique lines of PR.

@sbryngelson
Copy link
Member

sbryngelson commented Sep 9, 2024

This seems rather trivial, though admittedly, I haven't tried it myself. Can you give an example of what you've tried? It seems like diff file1.txt file2.txt should be sufficient. You could also ignore the condition that you should only show the diff of one of the files (e.g., PR).

Also:
https://superuser.com/questions/135171/listing-lines-from-just-one-file-in-diff

@okBrian
Copy link
Contributor Author

okBrian commented Sep 10, 2024

I've been using some variations of grep, diff, and even some with awk
First I would write each grep to a file like this
``
grep -F 'Wunused-variable' master.txt -B 4 >> mUnused.
grep -F 'Wunused-variable' pr.txt -B 4 >> prUnused.

Then some examples of commands I ran include these with some variation

grep -vxFf mUnused.txt prUnused.txt -B 4
awk 'NR==FNR{a[$0]=1;next}!a[$0]' mUnused.txt prUnused.txt
diff mUnused.txt prUnused


The main issue is that many of warnings will have paths like [stuff]/pr/[more stuff] and [stuff]/master/[more stuff] and the diff will pick that up as a difference which is obviously not right.

@sbryngelson
Copy link
Member

You can use sed to remove any instances of /pr/ or /master/ from the files altogether. that should fix it.

@sbryngelson
Copy link
Member

@okBrian what is the status of this?

@okBrian
Copy link
Contributor Author

okBrian commented Sep 11, 2024

Assuming I am reading the diff's correctly the warnings showing up are the exact same but they are on different line numbers??? Can you take a look at the most recent commit as I am just showing the diff for both and I am just confused why both are showing up even when they look the exact same.

@sbryngelson
Copy link
Member

Probably because you are not synced with master.

@okBrian
Copy link
Contributor Author

okBrian commented Sep 12, 2024

It works!!!

@sbryngelson
Copy link
Member

Is this ready to merge?

@okBrian
Copy link
Contributor Author

okBrian commented Sep 13, 2024

Yes this is ready to merge.

@sbryngelson sbryngelson merged commit 3cf7fb6 into MFlowCode:master Sep 13, 2024
22 of 23 checks passed
@sbryngelson sbryngelson mentioned this pull request Sep 15, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants