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

Fix a corner case for blank metric #580

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

Luni-4
Copy link
Collaborator

@Luni-4 Luni-4 commented Mar 25, 2021

This PR fixes blank metric when there are code lines and comments on the same line. Before of this PR, those kinds of lines were counted twice.

@Luni-4 Luni-4 requested a review from marco-c March 25, 2021 13:21
Copy link
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

Your test is a lucky case where this works, but it doesn't work in general. I think you actually have to subtract lines that are both ploc and cloc only once.
For example, try removing the blank line in python_negative_blank (the result is "blank == 2", while it should be 0). Or, add another comment at the end of another source line (again, the result is "blank == 2", while it should still be 1).

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Mar 26, 2021

Your test is a lucky case where this works, but it doesn't work in general. I think you actually have to subtract lines that are both ploc and cloc only once.
For example, try removing the blank line in python_negative_blank (the result is "blank == 2", while it should be 0). Or, add another comment at the end of another source line (again, the result is "blank == 2", while it should still be 1).

It should be fixed now

@Luni-4 Luni-4 changed the title Fix a corner case in counting blank metric Fix a corner case for blank metric Mar 26, 2021
@Luni-4 Luni-4 requested a review from marco-c March 26, 2021 17:43
src/metrics/loc.rs Outdated Show resolved Hide resolved
src/metrics/loc.rs Outdated Show resolved Hide resolved
@marco-c
Copy link
Collaborator

marco-c commented Mar 26, 2021

Can you test cases like:

   SOME CODE AND BLANK LINE
   printf("Ciao");  /* comment starts here
   continues here
   and ends here */
   MORE CODE
   SOME CODE AND BLANK LINE
   printf("Ciao");
   /* comment starts here
   continues here
   and ends here */
   MORE CODE
   SOME CODE AND BLANK LINE
   printf("Ciao");
   /* comment starts here
   continues here
   and ends here */ printf("Ciao;")
   MORE CODE

@Luni-4 Luni-4 force-pushed the fix-blank branch 2 times, most recently from b6e7319 to 9869865 Compare March 29, 2021 14:22
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Mar 29, 2021

Can you test cases like:

   SOME CODE AND BLANK LINE
   printf("Ciao");  /* comment starts here
   continues here
   and ends here */
   MORE CODE
   SOME CODE AND BLANK LINE
   printf("Ciao");
   /* comment starts here
   continues here
   and ends here */
   MORE CODE
   SOME CODE AND BLANK LINE
   printf("Ciao");
   /* comment starts here
   continues here
   and ends here */ printf("Ciao;")
   MORE CODE

Done

src/metrics/loc.rs Outdated Show resolved Hide resolved
src/metrics/loc.rs Outdated Show resolved Hide resolved
@marco-c
Copy link
Collaborator

marco-c commented Apr 6, 2021

Can you run the minimal tests script to see the difference before/after your patch on m-c? There might be cases we haven't thought of.

src/metrics/loc.rs Outdated Show resolved Hide resolved
src/metrics/loc.rs Outdated Show resolved Hide resolved
@Luni-4 Luni-4 force-pushed the fix-blank branch 2 times, most recently from 9b0108a to fad4836 Compare April 8, 2021 08:27
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Apr 8, 2021

@marco-c

I can see only improvements and I can't find any additional useful test, have a loot at it also, perhaps I've lost something important https://community-tc.services.mozilla.com/api/queue/v1/task/MIBNQ1TgTlaK_x2yDtPw_g/runs/0/artifacts/public%2Fjson-diffs-and-minimal-tests.tar.gz

@marco-c
Copy link
Collaborator

marco-c commented Apr 8, 2021

@Luni-4 looks good to me. Can you remove the test commit?

@marco-c marco-c merged commit 268e685 into mozilla:master Apr 8, 2021
@Luni-4 Luni-4 deleted the fix-blank branch April 8, 2021 11:52
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.

2 participants