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

Turn strict mode on by default in parsePatch #456

Closed
ExplodingCabbage opened this issue Jan 2, 2024 · 2 comments · Fixed by #508
Closed

Turn strict mode on by default in parsePatch #456

ExplodingCabbage opened this issue Jan 2, 2024 · 2 comments · Fixed by #508

Comments

@ExplodingCabbage
Copy link
Collaborator

Currently, if you pass a patch as a string to applyPatch and it has incorrect line counts in the hunk headers, we'll fail to apply it but won't throw an error to tell you that the patch itself was invalid. That seems bad; I spent a long time struggling to figure out why a patch wasn't applying in #238 because of this.

I don't really see any reason for strict mode not to be on by default, and suggest we make it so in the next major version.

@ExplodingCabbage
Copy link
Collaborator Author

While we're at it, we should probably also fix applyPatch to be able to apply a patch where the only thing wrong is the line counts. Either that or we should rip the strict option out of jsdiff entirely and make what we currently call "strict" mode be the only mode. It's misleading to have a non-strict mode if patches we parse in non-strict mode aren't actually usable by jsdiff.

@ExplodingCabbage
Copy link
Collaborator Author

Oh, man, when I make strict mode be always-on I see thirty test failures when running yarn test. I guess I need to figure out which of those (if any) are valid patches that strict mode was wrongly rejecting before, and which of them (if any) are invalid patches we were trying to parse or apply in our tests.

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

Successfully merging a pull request may close this issue.

1 participant