#1552 Add a git blame view of the files in the repo
Merged by pingou. Opened by pingou.
git_blame  into  master

Download 1552.patch
no initial comment

rebased

The title should be "View blame" or something like that

It would be good to document the arguments. It look me a bit of time to figure out "loc" was short for "line of code" and should be a (potentially multi-lined) string

It's a little weird to have two keys with the same value in this string replacement

you're right, this could be simplified

I assume this is actually when line == ''? I noticed that empty lines have no blame information, but that seems weird. Someone is to blame for blank lines.

hm, good call, we can like drop these 3 lines

:thumbsup:

Looks good!

One thing I like to do is have this sort of thing be its own test class with its own setup and teardown functions. It looks like there's 7-8 individual tests going on in this one test case, and it's nice for those to be broken up into a test case each so it's quickly apparent what is broken. It's also easier to add to, since you don't have to guess where in this long workflow to add additional assertions.

It's up to you, though.

Other than the title being from the "Raw" button and no blame information on empty lines, I'm satisfied with this. All the other comments are nitpicks or suggestions only.

:thumbsup:

rebased

Good catch

I've tried to improve the documentation on this

5 new commits added

  • Rework and simplify a little the logic in blame_loc
  • Fix the title of the blame button
  • Empty lines can also be blamed, so do not exclude them
  • Simplify the code in blame_loc, no need to use two variables for the same content
  • Better document what the blame_loc method is and does

For the tests I logged https://pagure.io/pagure/issue/1576 about restructuring them

I think I covered the rest, would you like to have a final look?

Looks good :thumbsup:

Thanks for the review, let's merge ! :)

Pull-Request has been merged by pingou

Metadata