-
Notifications
You must be signed in to change notification settings - Fork 291
Add diff.update to show preview of changes before running update #6080
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
|
hit me up either here or on discord to help as needed |
|
Hi @aryairani , I think this could use an initial review now. Thanks! |
…ce names; add diff.update to help
| -- | When True, always use raw strings (triple-quoted) for multiline text, | ||
| -- even when nested inside other expressions. This is useful for diff output | ||
| -- where we want actual newlines for better line-by-line diffing. | ||
| forceRawStrings :: !Bool |
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 remember offhand, should we not just have this on all the time?
I'm guessing that a multi-line string embedded in some other expression as a single line probably isn't wonderfully readable.
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.
yes, i was thinking that as well (and is indeed why this was added) but left it in in case there might be places where you want to do "\n\n" and just have that displayed as a single line (for instance), but ... no intelligent handling is currently implemented to decide on which alternative to use.
| -- we only use this syntax if we're not wrapped in something else, | ||
| -- to avoid possible round trip issues if the text ends at an odd column |
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 wonder what this "possible round trip issues if the text ends at an odd column" was; can you remember, @runarorama?
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.
We normally don't keep this file; stuff within unison-src/transcripts/idempotent/ doesn't produce an .output.md when run with transcripts unless there's an error.
(Running stack exec unison transcript unison-src/transcripts/idempotent/diff-update.md will produce the separate output file, but I recommend stack exec transcripts diff-update in this case.)
I'm gonna see if Github will let me delete it easily.
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.
Update: It did let me delete it easily.
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.
Thanks for the explanation - would it make sense to add a git ignore rule for *.output.md, or do we want to keep it as is to flag possible issues?
|
I'm gonna merge it, but we can still discuss the raw strings thing and consolidate the implementation later if appropriate. Thanks for the contribution! |
Overview
diff.updateallows a user to see inline code diffing within UCM before updating the codebase. This is most helpful for reviewing changes locally.A simple example (without color; deletions would be in red and additions in green):
Implementation approach and notes
The implementation reuses existing update infrastructure by leveraging
getNamespaceDependentsOfandhydrateRefsfrom
Cli.UpdateUtilsto fetch old definitions from the codebase. It employs an inline diff algorithm usingData.Algorithm.Diffto compute line-by-line differences between old and new definitions—including terms, structuraltypes, and abilities. The pretty-print environment includes names from the typechecked file (via
UF.typecheckedToNames)that shadow namespace names, ensuring new references resolve to names rather than hash abbreviations. Following the
existing
HandleInput/*.hspattern, the implementation uses a dedicatedDiffUpdate.hsmodule and exposes thefunctionality as a
diff-updatetool in the MCP server for AI agent access.Interesting/controversial decisions
The implementation chose an inline diff format over side-by-side comparison, using the
-/+prefix format (likegit diff) rather than a side-by-side layout, as this approach is more compact and familiar to developers.Test coverage
Have you included tests (which could be a transcript) for this change, or is it somehow covered by existing tests?
Would you recommend improving the test coverage (either as part of this PR or as a separate issue) or do you think it’s adequate?
Final checklist
.cabalfiles, make sure thepackage.yamlfiles are up-to-date instead.