Add some links for pre-commit and add default hooks #1651
Conversation
.pre-commit-config.yaml
Outdated
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v4.1.0 | ||
| hooks: | ||
| - id: trailing-whitespace |
There was a problem hiding this comment.
Hmmm... This seems to be enforcing that we do not have trailing whitespace. Does that mean that the final new line will be removed? I think having a final new line is very good practice when working with git as it makes you able to add a line below without seeing a change in the previous line etc.
I'd be unhappy about seeing the newlines be trimmed away at the end of files.
We use the end-of-file-fixer hook in most jupyterhub org repo's to ensure we get a new line at the end of the file. It is an autoformatter as well btw, so it automatically adds it if needed.
There was a problem hiding this comment.
trailing-whitespace just trims the redundant whitespaces on the line.
It doesn't remove final new line.
You can see that my PR passes the tests and doesn't almost change any code in the repository, so it's ok.
It works well with end-of-file-fixer as well, which is also added just one line before this.
There was a problem hiding this comment.
It will be redundant for Python files, and YAML, and I think also Markdown files thanks to black/prettier. Do you think we should retain it for the possible other file format that isn't managed by black and prettier?
I'd prefer to avoid adding it unless we do believe it can have use, otherwise it becomes additional complexity which in my mind almost always has some cost - if not only to rule out it wasn't related to some unknown behaviour observed.
There was a problem hiding this comment.
I think it should be simple and polished.
We have some other formats already - some text configuration files, ipynb, Makefile, gitignore, shell files.
I have just checked and it worked on requirements-dev.txt file and it fixed it as well.
So, it should be easy for developers as well.
I think we should give it a try and if it adds complexity or problems, then we will remove this check.
There was a problem hiding this comment.
It will always add complexity, but it certainly don't add much! Let's go for it!
What I mean about "It will always add complexity" is that anyone seeing this in the config and is tracking something that could have gone wrong for some reason, or want to learn and understand the code base as a whole - this is one more thing to rule out to cause problems and learn about. In that way, it would been less complicated to not have it at all.
But, yes! Let's go for it! You provided examples of files where it could be relevant!
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
No description provided.