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

Make diff information available about "new" lines #117

Open
loganharbour opened this issue Sep 23, 2021 · 7 comments
Open

Make diff information available about "new" lines #117

loganharbour opened this issue Sep 23, 2021 · 7 comments

Comments

@loganharbour
Copy link
Contributor

loganharbour commented Sep 23, 2021

From a code review prospective, we've found it valuable to have information about the lines of code that are edited (I refer to this here as "new"). I produce a table like this on PRs:

b79d3c #18890 d0b30f
Total Total +/- New
Rate 81.56% 81.56% +0.01% 100.00%
Hits 69452 69480 +28 29
Misses 15706 15707 +1 0

Where the new line coverage is generated with something like this:

differ = CoberturaDiff(base_cobertura, head_cobertura)
summary = {'hits': 0, 'misses': 0}

for file in differ.files():
    for line in differ.file_source(file):
        if line.status is not None and line.reason == 'line-edit':
            hits_misses_key = 'hits' if line.status else 'misses'
            summary[hits_misses_key] += 1

I'm curious if this has any place here instead - maybe at the very least as a helper that will produce something as such? I don't mind keeping this in our local generation, but the peace of mind of having it tested here instead is nice.

EDIT: maybe this has a better place in Discussions to get that kicked off?

@aconrad
Copy link
Owner

aconrad commented Sep 27, 2021

New lines seem like a useful metric to display. I can see how people can get a sense of how large the change is by looking at "New". What about you and your team, what was useful about it specifically?

I've never really used Discussions, I'm fine either way :)

@loganharbour
Copy link
Contributor Author

loganharbour commented Sep 30, 2021

Here's an example case:

16ff5b #18959 b38804
Total Total +/- New
Rate 86.05% 86.03% -0.01% 100.00%
Hits 23488 23459 -29 7
Misses 3808 3808 - 0

In this case, 36 hit lines were replaced with 7 lines. This is not immediately clear to some of our developers just by looking at the diff. There are a few cases where this is... useful, but this is the most immediate I've found. I'm definitely open to suggestions for better metrics/ways of presenting the data, as I suspect you are too.

@aconrad
Copy link
Owner

aconrad commented Sep 30, 2021

Works for me! If you want to add this to the output of pycobertura, go for it! 👍 I think it could be a useful metric to have by default.

@gro1m
Copy link
Contributor

gro1m commented Mar 13, 2022

@loganharbour Do you have any update here?

@loganharbour
Copy link
Contributor Author

loganharbour commented Mar 13, 2022

I've had this working without issue externally. I haven't found the time to port it over quite yet. Still need to write it such that it avoids duplicate effort when ran with the other default statistics.

If there's increased interest, I don't mind making it a higher priority!

@gro1m
Copy link
Contributor

gro1m commented Mar 16, 2022

Hi @loganharbour
I think the question came up more with regards to the upcoming major release 3.0.0.
Furthermore, as there have been quite some changes, it could be interesting to see your code sooner rather than later, so that you do not waste effort. In addition, my personal belief is that it is a good idea to submit a PR and get it merged within a year (which is still realistic in your case).

@gro1m
Copy link
Contributor

gro1m commented Mar 16, 2022

From an output perspective I wonder if it should go more in the direction of the git diff command, which would necessitate to have two tables or we could have the information side-by-side. Both these options would allow to represent all statistics and you might also see if a file is present in only one of the commits.

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

No branches or pull requests

3 participants