-
Notifications
You must be signed in to change notification settings - Fork 523
Allow more customisation of file headers in patches #641
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
…am quoting from the docs for the GNU version
|
I'm not expecting much, but out of curiosity, I'm gonna request another AI review before merging... |
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.
Pull request overview
This PR adds customization options for file headers in unified diff patches, allowing users to control which header elements are included. This resolves issue #624 by making jsdiff-generated patches compatible with tools like react-diff-view that require specific header formats.
Key changes:
- Introduces a
headerOptionsparameter that controls inclusion of index lines, underlines, and file headers - Exports three predefined constants (
INCLUDE_HEADERS,FILE_HEADERS_ONLY,OMIT_HEADERS) for common use cases - Updates
formatPatchand related functions to accept and propagate the new option
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/patch/create.ts | Defines HeaderOptions interface and constants; updates formatPatch to conditionally include headers; adds headerOptions to options interfaces for createPatch and createTwoFilesPatch |
| src/index.ts | Exports the three new header option constants (INCLUDE_HEADERS, FILE_HEADERS_ONLY, OMIT_HEADERS) |
| test/patch/create.js | Reorganizes existing header tests into a nested describe block; adds tests for the new header option constants |
| README.md | Documents the new headerOptions parameter with detailed explanations of each subfield and the predefined constants |
| release-notes.md | Adds release note describing the new feature and its motivation |
Comments suppressed due to low confidence (1)
src/index.ts:46
- The
HeaderOptionsinterface should be exported as a type so TypeScript consumers can use it when callingformatPatchor specifying theheaderOptionsfield in the options forcreatePatchandcreateTwoFilesPatch. Currently, consumers would need to import it directly from the internal module path, which is not ideal. Consider addingHeaderOptionsto the type import from './patch/create.js' and to the export type block.
import type {
StructuredPatchOptionsAbortable,
StructuredPatchOptionsNonabortable,
CreatePatchOptionsAbortable,
CreatePatchOptionsNonabortable
} from './patch/create.js';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const diffResult = createTwoFilesPatch('testFileName', 'testFileName', 'foo\n', 'bar\n', undefined, undefined, {headerOptions: OMIT_HEADERS}); | ||
| expect(diffResult).to.equal(expectedResult); | ||
| }); | ||
| }); |
Copilot
AI
Dec 31, 2025
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.
There is no direct test coverage for formatPatch with the new headerOptions parameter. While the feature is tested indirectly through createTwoFilesPatch tests, it would be beneficial to add explicit tests for formatPatch with different header options (INCLUDE_HEADERS, FILE_HEADERS_ONLY, OMIT_HEADERS) to ensure the feature works correctly when calling formatPatch directly. This is especially important since formatPatch is a public API that consumers might call directly with structured patch objects.
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.
@copilot open a new pull request to apply changes based on this feedback
|
@ExplodingCabbage I've opened a new pull request, #642, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
The extra detail in the Copilot review session page shows it caught some mistakes of mine that it didn't actually manage to post on the PR, for some reason:
Yep, good catch.
(also createPatch maybe?). Also a good catch. Not sure why this didn't get posted. |
…#642) * Initial plan * Add direct test coverage for formatPatch with headerOptions Co-authored-by: ExplodingCabbage <2358339+ExplodingCabbage@users.noreply.github.com> * Refactor: Extract common patch array to reduce duplication Co-authored-by: ExplodingCabbage <2358339+ExplodingCabbage@users.noreply.github.com> * Remove redundant test (covered entirely below) * Make test for multiple patches with OMIT_HEADERS demand sensible behaviour (which we don't yet provide) * Fix behaviour --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ExplodingCabbage <2358339+ExplodingCabbage@users.noreply.github.com> Co-authored-by: Mark Amery <markrobertamery@gmail.com>
|
Okay, you were actually useful this time, @copilot. Mind re-reviewing before I merge? |
|
@ExplodingCabbage I've opened a new pull request, #643, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Oh no, it was going so well. What's the crazy robot doing now? |
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - [#631](https://github.com/kpdecker/jsdiff/pull/631) - **fix support for using an `Intl.Segmenter` with `diffWords`**. This has been almost completely broken since the feature was added in v6.0.0, since it would outright crash on any text that featured two consecutive newlines between a pair of words (a very common case). | ||
| - [#635](https://github.com/kpdecker/jsdiff/pull/635) - **small tweaks to tokenization behaviour of `diffWords`** when used *without* an `Intl.Segmenter`. Specifically, the soft hyphen (U+00AD) is no longer considered to be a word break, and the multiplication and division signs (`×` and `÷`) are now treated as punctuation instead of as letters / word characters. | ||
| - [#641](https://github.com/kpdecker/jsdiff/pull/641) - **the format of file headers in `createPatch` etc. patches can now be customised somewhat**. It now takes a `headerOptions` option that can be used to disable the file headers entirely, or omit the `Index:` line and/or the underline. In particular, this was motivated by a request to make jsdiff patches compatible with react-diff-view, which they now are if produced with `headerOptions: FILE_HEADERS_ONLY`. |
Copilot
AI
Dec 31, 2025
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 "customised" uses British English spelling, while the document also uses American English spelling elsewhere (e.g., "behavior" on lines 33, 57, 63, 67, 73). Consider using "customized" (American English spelling) for consistency with parts of the document that use American spelling, or alternatively establish a consistent spelling convention throughout the document.
| - [#641](https://github.com/kpdecker/jsdiff/pull/641) - **the format of file headers in `createPatch` etc. patches can now be customised somewhat**. It now takes a `headerOptions` option that can be used to disable the file headers entirely, or omit the `Index:` line and/or the underline. In particular, this was motivated by a request to make jsdiff patches compatible with react-diff-view, which they now are if produced with `headerOptions: FILE_HEADERS_ONLY`. | |
| - [#641](https://github.com/kpdecker/jsdiff/pull/641) - **the format of file headers in `createPatch` etc. patches can now be customized somewhat**. It now takes a `headerOptions` option that can be used to disable the file headers entirely, or omit the `Index:` line and/or the underline. In particular, this was motivated by a request to make jsdiff patches compatible with react-diff-view, which they now are if produced with `headerOptions: FILE_HEADERS_ONLY`. |
Resolves the jsdiff side of #624 (although as I describe there I think there are probably related changes that are sensible for other tools to make).