Add Repository::blame_file()
#2153
Conversation
There was a problem hiding this comment.
Thanks a lot for tackling this! Note that I put the PR back into draft while CI is failing and there are questions.
This PR adds
Repository::blame_file(), reusing the existing feature flagblame. This is the minimal API I was able to come up with.
I think this works, the feature-flag usage is great, and the API is exactly what it should be - minimal and to the point. If there are other uses that need more control, they can easily use gix_blame directly, or one could offer a blame_opt() of sort that has more parameters.
Open questions
- The proposed API offers no way for the consumer to influence how
odb,cacheandresource_cacheget created. Is this the correct choice?
A good start, let's keep it simple until there is demand.
- As opposed to other APIs, e. g.
Repository::references, this API is notPlatform-based, but directly calls the underlying function. Is this something that should be changed/needs to be changed? What are the trade-offs involved?
It's not obvious to me how a platform would be helpful here, which probably means there should be none.
- There’s other APIs that provide a recorder-based API, such as
gix_diff::tree, allowing consumers to process the API’s output one-by-one while also allowing them to cancel it after each piece of output. Is that something we might want forgix_blame::file()and, if so, would we have to account for that in this initial version ofRepository::blame_file()?
We talked about the final-fancy version of gix-blame which would be far more complex, but while we don't have it, we should keep it simple.
- Is there anything that needs to be done in order for the tests in
gix/tests/gix/repository/blame.rsto be run on CI? I’m asking because the code I added is behind a feature flag.
Let's skip the tests, they seem redundant.
gix/src/repository/blame.rs
Outdated
| @@ -0,0 +1,43 @@ | |||
| #[cfg(feature = "blame")] | |||
There was a problem hiding this comment.
Since the whole module is behind a feature toggle, that toggle doesn't need to be used inside of it anymore.
gix/tests/gix/repository/blame.rs
Outdated
There was a problem hiding this comment.
I don't know if I'd test anything here as the method itself is really just a forwarding. Probably it's good to avoid duplication unless we know what specific thing we should test.
In theory, we could start adding doc-tests, then redundancy doesn't matter as the purpose is to inform the user, but it's by no means an obligation as they are quite cumbersome to make look nice.
There was a problem hiding this comment.
These tests indeed duplicate tests found elsewhere. I mainly added them to make sure that the whole gix::Repository::blame_file API could be used only relying on imports from gix. Feel free to remove them if you think they’re redundant!
gix/src/repository/blame.rs
Outdated
| pub enum Error { | ||
| #[error(transparent)] | ||
| CommitGraphIfEnabled(#[from] commit_graph_if_enabled::Error), | ||
| #[error(transparent)] | ||
| DiffResourceCache(#[from] diff_resource_cache::Error), | ||
| #[error(transparent)] | ||
| Blame(#[from] gix_blame::Error), | ||
| } |
There was a problem hiding this comment.
In the absence of custom 'connected' blame related types, I think this whole module should be in gix::repository::blame, with the error type 'where the other errors types are' - there is pattern for this.
Alternatively gix_blame::Outcome would have to be turned into a duplicate structure with a reference to repo: &Repository, which doesn't seem worth it - it's the bane of the gix crate as it always has to repeat whole types just to add some convenience.
There was a problem hiding this comment.
I’ve moved the enum, following the example of diff_tree_to_tree::Error and others. Was that correct?
cruessler
force-pushed
the
add-blame-file-on-repository
branch
from
75c4156 to
cdb1100
Compare
|
From my side, this PR is now ready for review! |
Byron
force-pushed
the
add-blame-file-on-repository
branch
from
d32ce8a to
3c54e5b
Compare
|
That's perfect, thanks so much! And yes, the test is definitely good to see how to access the return type, and I am grateful that you moved them instead of removing them as originally suggested. |
This PR adds
Repository::blame_file(), reusing the existing feature flagblame. This is the minimal API I was able to come up with.Open questions
odb,cacheandresource_cacheget created. Is this the correct choice?Repository::references, this API is notPlatform-based, but directly calls the underlying function. Is this something that should be changed/needs to be changed? What are the trade-offs involved?gix_diff::tree, allowing consumers to process the API’s output one-by-one while also allowing them to cancel it after each piece of output. Is that something we might want forgix_blame::file()and, if so, would we have to account for that in this initial version ofRepository::blame_file()?gix/tests/gix/repository/blame.rsto be run on CI? I’m asking because the code I added is behind a feature flag.