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

Unusable as 'pre-commit' hook, since it will always fail. #161

Open
aghdom opened this issue Mar 14, 2019 · 13 comments · May be fixed by #216
Open

Unusable as 'pre-commit' hook, since it will always fail. #161

aghdom opened this issue Mar 14, 2019 · 13 comments · May be fixed by #216

Comments

@aghdom
Copy link

aghdom commented Mar 14, 2019

Hi, I would like to point out a potential problem with pre-commit usage of doctoc. Since doctoc always updates files, which contain any headings, regardless of changes since the last run, it will always modify files and cause the pre-commit hook to fail and revert the changes.

This makes, in my opinion, doctoc unusable as a pre-commit hook as is, since it will cause every commit which would update a markdown file fail. The only option you are left with (and please correct me if I'm wrong) is to either skip pre-commit on that commit or remove doctoc from your pre-commit-config.yaml.

Suggestion

Since pre-commit aims more towards hooks which only check but, do not modify any files (such as linters, etc. ) the only way I see doctoc being used as a hook, would be if it only updated the files which had their structure changed since the last doctoc run or it had a mode in which it would simply run this check and fail, if some files needed updates. (Since both of them require this check to happen, it makes sense to implement it either way)

The way doctoc is working right now (and again, this is just my opinion and I'm open to being corrected) is not compatible with the direction pre-commit is heading. So in my humble opinion, the pre-commit integration in the current state is deprecated and it should either be addressed, or marked as suc. Again, I'm open for discussion.

@Pierre-Sassoulas
Copy link

I'm using doctoc with pre-commit without problems, maybe this issue is obsolete?

@smykhailov
Copy link

I have the same issue. I use husky hooks

  "husky": {
    "hooks": {
      "pre-commit": "doctoc .",
      "pre-push": "npm test"
    }
  }

I commit the changes:

image

after changes got committed my .md files got changed:

image

after it you commit this changes, the .md files got updated again and it goes to infinity...

@engineervix
Copy link

I'm using doctoc with pre-commit without problems, maybe this issue is obsolete?

@Pierre-Sassoulas are you able to share your configuration? Maybe there's something I'm missing, because, just as @aghdom pointed out, it's not working. I'm also using pre-commit BTW.

Here's my pre-commit-config.yaml:

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.1.0
    hooks:
      - id: check-added-large-files
        args: ["--maxkb=5000"]
      - id: end-of-file-fixer
      - id: check-case-conflict
      - id: detect-private-key
      - id: check-docstring-first
  - repo: https://github.com/psf/black
    rev: 19.10b0
    hooks:
      - id: black
        exclude: (.*)/migrations
  - repo: https://gitlab.com/PyCQA/flake8
    rev: 3.8.3
    hooks:
      - id: flake8
  - repo: https://github.com/prettier/prettier
    rev: 2.0.5
    hooks:
      - id: prettier
        exclude: >
          (?x)^(
              (.*)/vendors|
              (.*)/ico|
              package-lock.json|
              ^.+\.min\.(js|css)$|
              ^.+\.md$
          )$
  - repo: https://github.com/thlorenz/doctoc
    rev: v1.4.0
    hooks:
      - id: doctoc
  - repo: https://github.com/commitizen-tools/commitizen
    rev: v1.23.0
    hooks:
      - id: commitizen
        stages: [commit-msg]

@Pierre-Sassoulas
Copy link

Sorry, I don't have the config anymore, but it was the default on the latest release at the time of my comment.

@engineervix
Copy link

Sorry, I don't have the config anymore, but it was the default on the latest release at the time of my comment.

Alright, thanks @Pierre-Sassoulas. I have ended up using this Github Action

@adyaksaw
Copy link

I found out that doctoc precommit will work when I disable the core.autocrlf

@MapleCCC
Copy link

After investigation, I figure out the root cause. doctoc is intended to not modify the TOC when nothing has changed. The reason that some of you find doctoc always modifying TOC is due to the fact that doctoc splits the file content into an array of lines using the "\n" character as delimiter, while your files are probably using the "\r\n" as line endings. This is a bug when using doctoc on Windows platform. The solution is to change the following line:

var lines = content.split('\n')

@MapleCCC
Copy link

MapleCCC commented Oct 21, 2021

Please refer to the split-lines package (https://www.npmjs.com/package/split-lines) for a proper implementation of "splitting multiline string into an array of lines" that actually works on both UNIX and Windows environments.

@MapleCCC
Copy link

In short, we should change from
var lines = content.split('\n')
to
var lines = content.split(/\r?\n/)

@MapleCCC
Copy link

Besides this bug that happens when reading from files with "\r\n" line endings, another similar bug arises when writing to files with "\r\n" line endings, in which case we should have respected the line ending style of the original file.

@MapleCCC
Copy link

There are so many occurrences of the "\n" literal string in the source code that it's very infeasible and error prone to try to replace every occurrences of it with require('os').EOL. A better solution is to convert CRLF to LF immediately after fs.readFileSync(), and convert LF to CRLF immediately before fs.writeFileSync(), when we are on Windows.

MapleCCC added a commit to MapleCCC/doctoc that referenced this issue Oct 21, 2021
MapleCCC added a commit to MapleCCC/doctoc that referenced this issue Oct 21, 2021
@MapleCCC
Copy link

I create a PR (#216) that should fix this problem. Guys, please have a try, and if it solves your problem, go to the PR page to add a "+1" emoji, to express to the developer that the community finds the fix desirable.

@Mikaela
Copy link

Mikaela commented Apr 2, 2023

Another workaround is enforcing lf line endings for markdown files by creating a .gitattributes with lines such as *.md text eol=lf which will override gitconfig options such as core.autocrlf and has additional benefit of affecting everyone working with the repository.

It's also possible to * text=auto eol=lf to force git to autodetect whether file is text and if so make it use lf endings.

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 a pull request may close this issue.

7 participants