-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Comments
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 # 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] |
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 |
I found out that doctoc precommit will work when I disable the core.autocrlf |
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: Line 120 in ffbb04f
|
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. |
In short, we should change from |
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. |
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 |
… which causes insidious bug like thlorenz#161
… which causes insidious bug like thlorenz#161
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. |
…ributes handling for md/doctoc Ref: thlorenz/doctoc#161
Another workaround is enforcing lf line endings for markdown files by creating a It's also possible to |
Hi, I would like to point out a potential problem with
pre-commit
usage ofdoctoc
. Sincedoctoc
always updates files, which contain any headings, regardless of changes since the last run, it will always modify files and cause thepre-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 skippre-commit
on that commit or removedoctoc
from yourpre-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 seedoctoc
being used as a hook, would be if it only updated the files which had their structure changed since the lastdoctoc
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 directionpre-commit
is heading. So in my humble opinion, thepre-commit
integration in the current state is deprecated and it should either be addressed, or marked as suc. Again, I'm open for discussion.The text was updated successfully, but these errors were encountered: