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.

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

Document and test options to createPatch #345

Merged
merged 2 commits into from
May 6, 2022

Conversation

oBusk
Copy link
Contributor

@oBusk oBusk commented Feb 5, 2022

The documentation states that createPatch only supports context, when in actuality it passes the options on to the diffLines implementation.

This PR updates the documentation to clarify this and also adds tests to verify this behavior.

Since these methods are supposed to output a patch, it's arguable that the options doesn't make sense, but I'd argue that the ignoreWhitespace at least makes sense since outputting a patch is a pretty normalized way to display a diff and it makes sense to be able to use the existing options for it.

newLineIsToken is a bit weirder when creating "patches". I added docs and tests for that in separate commit so that it can be reverted if it's something that is not wanted.

oBusk added 2 commits February 5, 2022 04:22
I feel like this option is probably not that good since I supsect the patches it outputs doesn't really make any sense.
@kpdecker
Copy link
Owner

kpdecker commented May 6, 2022

LGTM, thank you for the contribution.

@kpdecker kpdecker merged commit fffd8a7 into kpdecker:master May 6, 2022
@oBusk oBusk deleted the patch-documentation branch August 8, 2022 20:42
@ExplodingCabbage
Copy link
Collaborator

newLineIsToken is a bit weirder when creating "patches". I added docs and tests for that in separate commit so that it can be reverted if it's something that is not wanted.

Yeah, IMO allowing newLineIsToken to be passed to patch functions at all is a misfeature. When there are actual changes to newline structure, the "patches" we get - like the one shown in the newlineIsToken: true test case added here - aren't valid patches and can't be applied to the original file by jsdiff or any other tool. I find it difficult to imagine this ever being useful to anyone but easy to imagine people getting caught out by it when they generate a "patch" with this option and then discover it can't be applied.

@ExplodingCabbage
Copy link
Collaborator

However, let me also thank you for the tests - without them, I would never have realised this feature existed or that I was unwittingly breaking it in my third commit at #535! I think I want to the feature, but now I can at least be more deliberate about doing so and document it in the release notes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants