-
Notifications
You must be signed in to change notification settings - Fork 19
feat: orchestrion diff #631
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
Conversation
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
56a2a56
to
d296497
Compare
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
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.
LGTM 🎉
I see some windows test failures because of the escaping issues.
var errs []error | ||
|
||
// Check if the diff command is available, exit early if not | ||
if _, err := exec.LookPath("diff"); err != nil { |
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.
Maybe in the subsequent PRs, we can consider git diff
facilities. e.g. I use fancy git diff setup maybe it is possible to use whatever is set for git.
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.
This would require to make sure all the options I currently pass are supported. We can create an issue about this for future planning 👍
internal/report/report.go
Outdated
} | ||
|
||
// When adding options, always make sure this is supported on all platforms. | ||
cmd := exec.Command("diff", |
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 assume our experiments with Go diff packages didn't go well?
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 tried basically all of them and even combining them but none of them have context window diffs which is a necessity when stdlib files makes multiple thousands lines long
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
- Coverage 68.63% 67.45% -1.19%
==========================================
Files 109 112 +3
Lines 7336 7715 +379
==========================================
+ Hits 5035 5204 +169
- Misses 1812 2001 +189
- Partials 489 510 +21
🚀 New features to boost your workflow:
|
Closes #571