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

Cognitive complexity metric for Java #850

Merged

Conversation

dburriss
Copy link
Contributor

@dburriss dburriss commented May 30, 2022

Last metric for #359

  • Implementation
  • All tests pass
  • Branch up-to-date
  • Cleanup

Tests and API have changed a fair bit since I started work on this so tracking some additional tasks:

  • update API usage to accommodate changes
  • redo tests with Insta
  • figure out why average not working (something to do with space functions I think)

@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-cognitive branch from e4845c0 to ca0cf88 Compare January 13, 2023 13:32
@marco-c
Copy link
Collaborator

marco-c commented Apr 5, 2023

@dburriss any plans to finish this?

@dburriss
Copy link
Contributor Author

Hi @marco-c
Sorry was job hunting and then ramping up so didn't have much energy for this. I'll try dive into this again tomorrow morning now that things have settled.

@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-cognitive branch from 49ce25c to 1b3821c Compare June 2, 2023 05:28
@Luni-4
Copy link
Collaborator

Luni-4 commented Jun 15, 2023

Hello @dburriss, thank you for your hard work!

I think that if you insert test methods inside a class, you would get the correct average value. Something like this:

class X {
     public static void print(boolean a, boolean b, boolean c, boolean d){  // +1
         if (a && !(b && c)) { // +3 (+1 &&, +1 &&)
             printf(\"test\");
         }
     }
}

@dburriss
Copy link
Contributor Author

Hi @Luni-4
Thanks for the suggestion. I'll give that a try.

@Luni-4
Copy link
Collaborator

Luni-4 commented Jun 16, 2023

Hi @Luni-4 Thanks for the suggestion. I'll give that a try.

It worked!

@dburriss dburriss force-pushed the dburriss/feature/issue-359/java-cognitive branch from 438bc74 to 9ef21cf Compare June 24, 2023 07:03
@dburriss dburriss marked this pull request as ready for review June 24, 2023 07:44
@dburriss
Copy link
Contributor Author

@marco-c @Luni-4 It looks like this is finally ready for a second pair of eyes.

Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Thank you @dburriss! Just simple things from my side:

  • we need to remove the Cargo.lock file since it is no longer necessary
  • we need to replace Boolean with boolean in Java functions

src/metrics/cognitive.rs Outdated Show resolved Hide resolved
src/metrics/cognitive.rs Outdated Show resolved Hide resolved
src/metrics/cognitive.rs Outdated Show resolved Hide resolved
src/metrics/cognitive.rs Outdated Show resolved Hide resolved
@dburriss
Copy link
Contributor Author

@Luni-4 thanks for the review. I have made the requested improvements.

Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Thank you @dburriss! Just simple things again

src/metrics/cognitive.rs Outdated Show resolved Hide resolved
src/metrics/cognitive.rs Outdated Show resolved Hide resolved
src/metrics/cognitive.rs Outdated Show resolved Hide resolved
src/metrics/cognitive.rs Show resolved Hide resolved
src/metrics/cognitive.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Ok, last change and we are ready to merge in!

src/metrics/cognitive.rs Outdated Show resolved Hide resolved
@dburriss
Copy link
Contributor Author

dburriss commented Jun 27, 2023

@marco-c the task expired before a worker was made available for the checks. Should I push another commit to trigger or is there an issue (and trying again could make it worse).

@marco-c
Copy link
Collaborator

marco-c commented Jun 27, 2023

I retriggered it manually and it passed.

Luni-4
Luni-4 previously approved these changes Jun 27, 2023
Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

It's fine for me! Thanks a lot @dburriss!

@dburriss
Copy link
Contributor Author

@marco-c the steps that had not run passed but the rest were in an exception state due to timeout exceeded. Triggered a check with a commit.
@Luni-4 I updated a comment to get the check to run. You may want to check it since I've got those wrong before :p

@dburriss dburriss requested a review from Luni-4 June 28, 2023 04:59
Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

@dburriss Your last commit is fine as well for me! Thank you!

@Luni-4 Luni-4 merged commit 88d957e into mozilla:master Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants