-
Notifications
You must be signed in to change notification settings - Fork 18
feat: word diffs #33
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.
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
feat: word diffs #33
Changes from 12 commits
969dde8
a625abc
1bf3ccf
3dba63f
2682999
a175626
fbff98b
ae627f5
acdad54
337bbdb
236ae42
4c14911
83fe6bd
b24e52c
9290066
30d2f41
936bbcf
0194b50
d042243
cd20497
d193772
7724909
e92b3c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,7 +141,10 @@ | |
| use std::ops::Range; | ||
| use std::slice; | ||
|
|
||
| use crate::util::{strip_common_postfix, strip_common_prefix}; | ||
| use crate::{ | ||
| sources::words, | ||
| util::{strip_common_postfix, strip_common_prefix}, | ||
| }; | ||
|
|
||
| pub use crate::slider_heuristic::{ | ||
| IndentHeuristic, IndentLevel, NoSliderHeuristic, SliderHeuristic, | ||
|
|
@@ -389,6 +392,45 @@ impl Hunk { | |
| pub fn is_pure_removal(&self) -> bool { | ||
| self.after.is_empty() | ||
| } | ||
|
|
||
| /// Performs a word-diff of the hunk | ||
| pub fn word_diff<'a>( | ||
| &self, | ||
| input: &InternedInput<&'a str>, | ||
| diff_input: &mut InternedInput<&'a str>, | ||
| diff: &mut Diff, | ||
| ) { | ||
| let Hunk { before, after } = self.clone(); | ||
|
Byron marked this conversation as resolved.
|
||
| diff_input.update_before( | ||
| before | ||
| .map(|index| input.before[index as usize]) | ||
| .map(|token| input.interner[token]) | ||
| .flat_map(|line| words(line)), | ||
| ); | ||
| diff_input.update_after( | ||
| after | ||
| .map(|index| input.after[index as usize]) | ||
| .map(|token| input.interner[token]) | ||
| .flat_map(|line| words(line)), | ||
| ); | ||
| diff.removed.clear(); | ||
| diff.removed.resize(diff_input.before.len(), false); | ||
| diff.added.clear(); | ||
| diff.added.resize(diff_input.after.len(), false); | ||
| if self.is_pure_removal() { | ||
| diff.removed.fill(true); | ||
| } else if self.is_pure_insertion() { | ||
| diff.added.fill(true); | ||
| } else { | ||
| diff.compute_with( | ||
| Algorithm::Myers, | ||
|
KnorpelSenf marked this conversation as resolved.
|
||
| &diff_input.before, | ||
| &diff_input.after, | ||
| diff_input.interner.num_tokens(), | ||
| ); | ||
| diff.postprocess_no_heuristic(diff_input); | ||
| } | ||
| } | ||
|
||
| } | ||
|
|
||
| /// Yields all [`Hunk`]s in a file in monotonically increasing order. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,13 @@ pub fn lines(data: &str) -> Lines<'_> { | |||||
| Lines(ByteLines(data.as_bytes())) | ||||||
| } | ||||||
|
|
||||||
| /// Returns a [`TokenSource`] that uses the words in `data` as Tokens. A word is | ||||||
| /// a sequence of alphanumeric characters as determined by | ||||||
| /// `char::is_alphanumeric`. Any other characters are their own word. | ||||||
| pub fn words(data: &str) -> Words<'_> { | ||||||
| Words(data) | ||||||
| } | ||||||
|
|
||||||
| /// Returns a [`TokenSource`] that uses the lines in `data` as Tokens. The newline | ||||||
| /// separator (`\r\n` or `\n`) is included in the emitted tokens. This means that changing | ||||||
| /// the newline separator from `\r\n` to `\n` (or omitting it fully on the last line) is | ||||||
|
|
@@ -79,6 +86,53 @@ impl<'a> TokenSource for Lines<'a> { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// A [`TokenSource`] that returns the words of a string as tokens. See | ||||||
| /// [`words`] for details. | ||||||
| #[derive(Clone, Copy, PartialEq, Eq)] | ||||||
| pub struct Words<'a>(&'a str); | ||||||
|
|
||||||
| impl<'a> Iterator for Words<'a> { | ||||||
| type Item = &'a str; | ||||||
|
|
||||||
| fn next(&mut self) -> Option<Self::Item> { | ||||||
| if self.0.is_empty() { | ||||||
| return None; | ||||||
| } | ||||||
|
|
||||||
| let initial = self.0.chars().next().unwrap(); | ||||||
| let word_len = if initial == ' ' { | ||||||
| self.0 | ||||||
| .char_indices() | ||||||
| .find(|(_, c)| *c != ' ') | ||||||
| .map_or(self.0.len(), |(index, _)| index) | ||||||
| } else if initial.is_alphanumeric() { | ||||||
|
KnorpelSenf marked this conversation as resolved.
|
||||||
| self.0 | ||||||
| .char_indices() | ||||||
| .find(|(_, c)| !c.is_alphanumeric()) | ||||||
|
KnorpelSenf marked this conversation as resolved.
Outdated
|
||||||
| .map_or(self.0.len(), |(index, _)| index) | ||||||
| } else { | ||||||
| initial.len_utf8() | ||||||
| }; | ||||||
|
|
||||||
| let (word, rem) = self.0.split_at(word_len); | ||||||
| self.0 = rem; | ||||||
| Some(word) | ||||||
| } | ||||||
| } | ||||||
| impl<'a> TokenSource for Words<'a> { | ||||||
| type Token = &'a str; | ||||||
|
|
||||||
| type Tokenizer = Self; | ||||||
|
|
||||||
| fn tokenize(&self) -> Self::Tokenizer { | ||||||
| *self | ||||||
| } | ||||||
|
|
||||||
| fn estimate_tokens(&self) -> u32 { | ||||||
| (self.0.len() / 3) as u32 | ||||||
|
||||||
| (self.0.len() / 3) as u32 | |
| (self.0.len() / 5) as u32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about this one, but it seems that heuristics aren't always right and maybe there is a way to make it configurable?
Maybe this word-diff is also so tuned to Latin text that we might say it in the function, i.e. word_diff to latin_word_diff.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ use expect_test::{expect, expect_file}; | |
| // use git_repository as git; | ||
|
|
||
| use crate::intern::InternedInput; | ||
| use crate::sources::words; | ||
| use crate::unified_diff::BasicLineDiffPrinter; | ||
| use crate::{Algorithm, Diff, UnifiedDiffConfig}; | ||
|
|
||
|
|
@@ -320,6 +321,135 @@ i | |
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn hunk_word_diff_pure() { | ||
| let before = r#"fn foo() -> Bar{ | ||
| let mut foo = 2.0; | ||
| foo *= 100 / 2; | ||
| }"#; | ||
| let after = r#"fn foo() -> Bar{ | ||
| let mut foo = 2.0; | ||
| foo *= 100 / 2; | ||
| println("hello world") | ||
| }"#; | ||
| let mut input = InternedInput::new(before, after); | ||
| for algorithm in Algorithm::ALL { | ||
| let mut diff_input = InternedInput::default(); | ||
| let mut d = Diff::default(); | ||
|
||
|
|
||
| println!("{algorithm:?}"); | ||
|
|
||
| let mut diff = Diff::compute(algorithm, &input); | ||
| diff.postprocess_lines(&input); | ||
|
|
||
| let mut hunks = diff.hunks(); | ||
| let hunk = hunks.next().expect("missing first hunk"); | ||
| hunk.word_diff(&input, &mut diff_input, &mut d); | ||
| let mut h = d.hunks(); | ||
| let first = h.next().expect("missing first inner hunk"); | ||
| assert!(first.is_pure_insertion()); | ||
| assert_eq!(first.before, 0..0); | ||
| assert_eq!( | ||
| first.after, | ||
| 0..words(" println(\"hello world\")\n").count() as u32 | ||
| ); | ||
| assert_eq!(h.next(), None); | ||
| assert_eq!(hunks.next(), None); | ||
|
|
||
| swap(&mut input.before, &mut input.after); | ||
|
|
||
| let mut diff = Diff::compute(algorithm, &input); | ||
| diff.postprocess_lines(&input); | ||
|
|
||
| let mut hunks = diff.hunks(); | ||
| let hunk = hunks.next().expect("missing first hunk"); | ||
| hunk.word_diff(&input, &mut diff_input, &mut d); | ||
| let mut h = d.hunks(); | ||
| let first = h.next().expect("missing first inner hunk"); | ||
| assert!(first.is_pure_removal()); | ||
| assert_eq!( | ||
| first.before, | ||
| 0..words(" println(\"hello world\")\n").count() as u32 | ||
| ); | ||
| assert_eq!(first.after, 0..0); | ||
| assert_eq!(h.next(), None); | ||
| assert_eq!(hunks.next(), None); | ||
|
|
||
| swap(&mut input.before, &mut input.after); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn hunk_word_diff_modify() { | ||
| let before = r#"fn foo() -> Bar { | ||
| let mut foo = 2.0; | ||
| foo *= 100 / 2; | ||
| }"#; | ||
| let after = r#"fn foo() -> Bar { | ||
| let mut foo = 3.0 * 2.0; | ||
| foo += 100 / 2; | ||
| }"#; | ||
| let mut input = InternedInput::new(before, after); | ||
| for algorithm in Algorithm::ALL { | ||
| let mut diff_input = InternedInput::default(); | ||
| let mut d = Diff::default(); | ||
|
||
|
|
||
| println!("{algorithm:?}"); | ||
|
|
||
| let mut diff = Diff::compute(algorithm, &input); | ||
| diff.postprocess_lines(&input); | ||
|
|
||
| let mut hunks = diff.hunks(); | ||
| let hunk = hunks.next().expect("missing first hunk"); | ||
| hunk.word_diff(&input, &mut diff_input, &mut d); | ||
| let mut h = d.hunks(); | ||
| let first = h.next().expect("missing first inner hunk"); | ||
| assert!(first.is_pure_insertion()); | ||
| let off = words(" let mut foo = ").count() as u32; | ||
| assert_eq!(first.before, off..off); | ||
| let ins = words("3.0 * ").count() as u32; | ||
| assert_eq!(first.after, off..ins + off); | ||
| let second = h.next().expect("missing second inner hunk"); | ||
| let off = words( | ||
| r#" let mut foo = 2.0; | ||
| foo "#, | ||
| ) | ||
| .count() as u32; | ||
| assert_eq!(second.before, off..1 + off); | ||
| assert_eq!(second.after, ins + off..1 + ins + off); | ||
| assert_eq!(h.next(), None); | ||
| assert_eq!(hunks.next(), None); | ||
|
|
||
| swap(&mut input.before, &mut input.after); | ||
|
|
||
| let mut diff = Diff::compute(algorithm, &input); | ||
| diff.postprocess_lines(&input); | ||
|
|
||
| let mut hunks = diff.hunks(); | ||
| let hunk = hunks.next().expect("missing first hunk"); | ||
| hunk.word_diff(&input, &mut diff_input, &mut d); | ||
| let mut h = d.hunks(); | ||
| let first = h.next().expect("missing first inner hunk"); | ||
| assert!(first.is_pure_removal()); | ||
| let off = words(" let mut foo = ").count() as u32; | ||
| let rem = words("3.0 * ").count() as u32; | ||
| assert_eq!(first.before, off..rem + off); | ||
| assert_eq!(first.after, off..off); | ||
| let second = h.next().expect("missing second inner hunk"); | ||
| let off = words( | ||
| r#" let mut foo = 2.0; | ||
| foo "#, | ||
| ) | ||
| .count() as u32; | ||
| assert_eq!(second.before, rem + off..1 + rem + off); | ||
| assert_eq!(second.after, off..1 + off); | ||
| assert_eq!(h.next(), None); | ||
| assert_eq!(hunks.next(), None); | ||
|
|
||
| swap(&mut input.before, &mut input.after); | ||
| } | ||
| } | ||
|
|
||
| pub fn project_root() -> PathBuf { | ||
| let dir = env!("CARGO_MANIFEST_DIR"); | ||
| let mut res = PathBuf::from(dir); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word_diff API requires callers to pass mutable references to diff_input and diff that are populated by this function. This design is not intuitive and error-prone because:
Consider either:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While copilot is right in principle, I think compete documentation on parameters could explain why they need to be mutable.
Docs are definitely needed here.