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

Fix several bugs due to line endings not properly handled on Windows platform #216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MapleCCC
Copy link

In a nut shell, this PR fixes several bugs:

  1. Fix the bug that files on Windows always get modified by doctoc even when nothing has changed, and ultimately leading to non-usability of doctoc as pre-commit hook, elaborated on issue Unusable as 'pre-commit' hook, since it will always fail. #161.

  2. Fix the bug that files on Windows, after modified by doctoc, inadvertently get undesirable mixed line endings.

  3. Fix the bug that files on Windows with a TOC that has no title, are inferred by doctoc to has a TOC title that contains a single character "\r", which ultimately leads to extra line breaks.

Close #161.

Copy link
Collaborator

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, and sorry for the slow reply!

I worry a little about this being the default behavior always. Perhaps there are Windows users out there who already have this problem solved. Or that as a Windows user perhaps they're still using the regular \r in their editors.

So my impression right now is that it isn't safe to turn on for everyone.

If you wanted, you could create a flag to enable the behavior, something like --useCRLFEndings or something. Then people could opt into it if that's their desired line endings.

@@ -16,6 +17,28 @@ function cleanPath(path) {
return homeExpanded.replace(/\s/g, '\\ ');
}

function readFile(path, encoding) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a name like xFileWithCRLFHandling would be more self-documenting.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (4)

doctoc.js:20

  • [nitpick] The function name 'readFile' is generic. Consider renaming it to 'readFileWithLineEndingConversion' for clarity.
function readFile(path, encoding) {

doctoc.js:32

  • [nitpick] The function name 'writeFile' is generic. Consider renaming it to 'writeFileWithLineEndingConversion' for clarity.
function writeFile(path, data, encoding) {

doctoc.js:20

  • Ensure that the new behavior of converting line endings in 'readFile' is covered by tests.
function readFile(path, encoding) {

doctoc.js:32

  • Ensure that the new behavior of converting line endings in 'writeFile' is covered by tests.
function writeFile(path, data, encoding) {
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.

Unusable as 'pre-commit' hook, since it will always fail.
2 participants