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.
line == ''
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.
Good catch
I've tried to improve the documentation on this
5 new commits added
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