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.

Sign up for GitHub

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

Jump to bottom

surf: Add stats property to each DiffContent::Plain #145

Merged
merged 1 commit into from
Oct 2, 2023
Merged

surf: Add stats property to each DiffContent::Plain #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions radicle-surf/src/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ pub enum DiffContent {
#[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))]
Plain {
hunks: Hunks<Modification>,
stats: FileStats,
eof: EofNewLine,
},
Empty,
Expand All @@ -296,10 +297,18 @@ pub enum DiffContent {
impl DiffContent {
pub fn eof(&self) -> Option<EofNewLine> {
match self {
Self::Plain { hunks: _, eof } => Some(eof.clone()),
Self::Plain { eof, .. } => Some(eof.clone()),
_ => None,
}
}

pub fn stats(&self) -> Option<&FileStats> {
match &self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why are we calculating the stats here if it's contained in DiffContent::Plain? 🤔

I think we want to see if git2 can provide the stats per file and get them from there when we create the DiffContent (in the TryFrom impls under diff::git).

So we should just match and return:

DiffContent::Plain { stats, .. } => Some(stats),
DiffContent::Empty => None,
DiffContent::Binary => None,

I'd return an Option because stats don't make sense for the other two.

Copy link
Member Author

@sebastinez sebastinez Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that works, seems like it did the job twice 😁

Done ✅

DiffContent::Plain { stats, .. } => Some(stats),
DiffContent::Empty => None,
DiffContent::Binary => None,
}
}
}

/// File mode in a diff.
Expand Down Expand Up @@ -371,23 +380,23 @@ impl Serialize for FileDiff {
match &self {
FileDiff::Added(x) => {
state.serialize_field("path", &x.path)?;
state.serialize_field("diff", &x.diff)?
state.serialize_field("diff", &x.diff)?;
},
FileDiff::Deleted(x) => {
state.serialize_field("path", &x.path)?;
state.serialize_field("diff", &x.diff)?
state.serialize_field("diff", &x.diff)?;
},
FileDiff::Modified(x) => {
state.serialize_field("path", &x.path)?;
state.serialize_field("diff", &x.diff)?;
},
FileDiff::Moved(x) => {
state.serialize_field("oldPath", &x.old_path)?;
state.serialize_field("newPath", &x.new_path)?
state.serialize_field("newPath", &x.new_path)?;
},
FileDiff::Copied(x) => {
state.serialize_field("oldPath", &x.old_path)?;
state.serialize_field("newPath", &x.new_path)?
state.serialize_field("newPath", &x.new_path)?;
},
}
state.end()
Expand All @@ -411,6 +420,16 @@ impl Serialize for Diff {
}
}

/// Statistics describing a particular [`FileDiff`].
#[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))]
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
pub struct FileStats {
/// Get the total number of additions in a [`FileDiff`].
pub additions: usize,
/// Get the total number of deletions in a [`FileDiff`].
pub deletions: usize,
}

/// Statistics describing a particular [`Diff`].
#[cfg_attr(feature = "serde", derive(Serialize), serde(rename_all = "camelCase"))]
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
Expand Down
15 changes: 15 additions & 0 deletions radicle-surf/src/diff/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use super::{
DiffFile,
EofNewLine,
FileMode,
FileStats,
Hunk,
Hunks,
Line,
Expand Down Expand Up @@ -152,6 +153,8 @@ impl TryFrom<git2::Patch<'_>> for DiffContent {
let mut hunks = Vec::new();
let mut old_missing_eof = false;
let mut new_missing_eof = false;
let mut additions = 0;
let mut deletions = 0;

for h in 0..patch.num_hunks() {
let (hunk, hunk_lines) = patch.hunk(h)?;
Expand All @@ -166,11 +169,19 @@ impl TryFrom<git2::Patch<'_>> for DiffContent {
old_missing_eof = true;
continue;
},
git2::DiffLineType::Addition => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you are calculating them here. I checked DiffFile too and there were no individual stats.

Copy link
Member Author

@sebastinez sebastinez Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want the individual stats to show up in the DiffFile since we are interested in the changes introduced by a specific diff which is specified by DiffContent.
While the DiffFile only relates to a specific blob.

additions += 1;
},
git2::DiffLineType::Deletion => {
deletions += 1;
},
git2::DiffLineType::AddEOFNL => {
additions += 1;
old_missing_eof = true;
continue;
},
git2::DiffLineType::DeleteEOFNL => {
deletions += 1;
new_missing_eof = true;
continue;
},
Expand All @@ -194,6 +205,10 @@ impl TryFrom<git2::Patch<'_>> for DiffContent {
};
Ok(DiffContent::Plain {
hunks: Hunks(hunks),
stats: FileStats {
additions,
deletions,
},
eof,
})
}
Expand Down
33 changes: 33 additions & 0 deletions radicle-surf/t/src/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use radicle_surf::{
EofNewLine,
FileDiff,
FileMode,
FileStats,
Hunk,
Line,
Modification,
Expand Down Expand Up @@ -48,6 +49,10 @@ fn test_initial_diff() -> Result<(), Error> {
new: 1..2,
}]
.into(),
stats: FileStats {
additions: 1,
deletions: 0,
},
eof: EofNewLine::default(),
},
new: DiffFile {
Expand Down Expand Up @@ -99,6 +104,10 @@ fn test_diff_file() -> Result<(), Error> {
new: 1..3,
}]
.into(),
stats: FileStats {
additions: 2,
deletions: 1
},
eof: EofNewLine::default(),
},
old: DiffFile {
Expand Down Expand Up @@ -137,6 +146,10 @@ fn test_diff() -> Result<(), Error> {
new: 1..3,
}]
.into(),
stats: FileStats {
additions: 2,
deletions: 1
},
eof: EofNewLine::default(),
},
old: DiffFile {
Expand Down Expand Up @@ -238,6 +251,10 @@ fn test_diff_serde() -> Result<(), Error> {
"old": { "start": 0, "end": 0 },
"new": { "start": 1, "end": 3 },
}],
"stats": {
"additions": 2,
"deletions": 0,
},
"eof": "noneMissing",
},
"new": {
Expand Down Expand Up @@ -295,6 +312,10 @@ fn test_diff_serde() -> Result<(), Error> {
"old": { "start": 1, "end": 8 },
"new": { "start": 0, "end": 0 },
}],
"stats": {
"additions": 0,
"deletions": 7,
},
"eof": "noneMissing",
},
}],
Expand All @@ -310,6 +331,10 @@ fn test_diff_serde() -> Result<(), Error> {
"diff": {
"eof": "noneMissing",
"hunks": [],
"stats": {
"additions": 0,
"deletions": 0,
},
"type": "plain",
},
"new": {
Expand Down Expand Up @@ -350,6 +375,10 @@ fn test_diff_serde() -> Result<(), Error> {
"old": { "start": 1, "end": 3 },
"new": { "start": 1, "end": 3 },
}],
"stats": {
"additions": 2,
"deletions": 2
},
"eof": "noneMissing",
},
"new": {
Expand Down Expand Up @@ -553,6 +582,10 @@ index 3f69208f3..cbc843c82 100644
},
},
],
"stats": {
"additions": 7,
"deletions": 5
},
"type": "plain",
},
"new": {
Expand Down
Loading