search

radicle-dev / radicle-surf

A code browsing library for VCS file systems.
Other
32 stars 11 forks source link

Add a associated function to `surf::diff::Diff` that allows us to generate `source::commit::Stats` over two commits #205

Open sebastinez opened 2 years ago

sebastinez commented 2 years ago

Hello everyone,

So I wanted to propose here, that we should add an associated function to surf::diff::Diff that would allow us to generate a source::commit::Stats.

Currently it is possible to generate a surf::diff::Diff through e.g. https://github.com/radicle-dev/radicle-surf/blob/77b1905ba38fc9c95c01c0ed09d4d495548eacc2/surf/src/vcs/git.rs#L543 But we don't have any function that would calculate the amount of additions and deletions that happened between those commits.

I'm thinking we should be able to convert the additions, deletions loops from here https://github.com/radicle-dev/radicle-surf/blob/77b1905ba38fc9c95c01c0ed09d4d495548eacc2/source/src/commit.rs#L135 into the mentioned associated function.

FintanH commented 2 years ago

Ya, that makes sense to me. So we would actually move the source::commit::Stats type into surf::diff. I'm wondering if it would make sense to calculate the Stats while building the Diff, so it just becomes associated data rather than an associated computation. The proposed function would just be an accessor then :)

sebastinez commented 2 years ago

I'm wondering if it would make sense to calculate the Stats while building the Diff.

Yeah that sounds great to me 👍 Would provide a PR for that

FintanH commented 2 years ago

Thanks, @sebastinez! Appreciate the suggestion and the follow-up :blush:

sebastinez commented 2 years ago

I'm wondering if it would make sense to calculate the Stats while building the Diff, so it just becomes associated data rather than an associated computation.

I did some more thinking maybe if we leave it as an associated computation, one could opt in if needed. Also I like the structure of the surf::diff::Diff and wouldn't change it.

FintanH commented 2 years ago

I did some more thinking maybe if we leave it as an associated computation, one could opt in if needed.

What's the con of not having it?

I would think it's cheap to carry in the diff itself as a sub-struct since it's only two u64's. Is there something I'm missing?

cloudhead commented 2 years ago

Yeah since it's really cheap I don't think it's so useful to be able to opt-out?