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

Correct the count of lines. #266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

eyal0
Copy link

@eyal0 eyal0 commented Nov 27, 2019

The number of lines in a file is usually computed at the number of newlines in the file minus 1, as described here. I also confirmed this by running wc on linux with a file that is:

  • empty
  • non-empty but with no newlines
  • with newlines and no trailing newline
  • with newlines and with a trailing newline

I noticed this when I saw that the number of lines reported in the file list on coveralls didn't match the number of lines actually in the file.

The number of lines in a file is usually computed at the number of newlines in the file minus 1, as described [here](https://codereview.stackexchange.com/questions/131143/counting-newlines-in-a-file).  I also confirmed this by running `wc` on linux with a file that is:

* empty
* non-empty but with no newlines
* with newlines and no trailing newline
* with newlines and with a trailing newline

I noticed this when I saw that the number of lines reported in the file list on coveralls didn't match the number of lines actually in the file.
@jtwebman
Copy link

jtwebman commented Mar 6, 2022

Since this library doesn't seem to be supported anymore I fix a bunch of things on a fork if you want to check it out and are still pulling the library into your packages: https://github.com/jtwebman/coveralls-next I would be willing to pull this change in.

@eyal0
Copy link
Author

eyal0 commented Mar 7, 2022

I'm using the coveralls github action mostly: https://github.com/marketplace/actions/coveralls-github-action .

You should take this change, though, because it makes the code more correct.

@jtwebman
Copy link

jtwebman commented Mar 8, 2022

Good call though your change here was in the wrong place. The split makes the array of lines. We needed it on the length call. Made the change in this PR for coveralls-next. jtwebman/coveralls-next#3

@jtwebman
Copy link

jtwebman commented Mar 8, 2022

Now that I look at the change though and test it out on that project it could be dangerous as people could set up their project to not leave a blank line on the end. If that line has no code then it will not be counted for code coverage. So the safe bet is to keep it as if there was code on those lines this code will error as the LCOV format could have a line with those values.

@eyal0
Copy link
Author

eyal0 commented Mar 8, 2022

Now that I look at the change though and test it out on that project it could be dangerous as people could set up their project to not leave a blank line on the end. If that line has no code then it will not be counted for code coverage. So the safe bet is to keep it as if there was code on those lines this code will error as the LCOV format could have a line with those values.

Like I wrote above, I tested it against wc's concept of number of lines. A file with no newlines has no lines in it.

└──> wc /dev/null
      0       0       0 /dev/null
└──> echo "hi" | wc
      1       1       3
└──> echo -n "hi" | wc
      0       1       2

But what makes sense for coveralls and what works for wc might be different. Ah well.

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