PreCommit should fix mixed-line-endings automatically / Add .gitattribute

[julie777]

Discussed in #337

^{Originally posted by CarliJoy December 14, 2020}

Description of your problem

Copy stuff from somewhere to PyCharm from a source with different line ending.

This will prevent me from committing at all.
Users with not so much experience (not everybody who writes python packages is a computer geek) will have a hard time getting the commit to work.
It would be nice if the setting is more forgiving, so there is an error but an auto fix.

Please provide any additional information below.
Suggestion: Change the .pre-commit-config.yaml to

  - id: mixed-line-ending
    args: ['--fix=auto']

I don't know if that catches all edge cases especially working with autoclrf.
Maybe it would be nice to introduce a .gitattribute instead?
See also: https://stackoverflow.com/questions/2825428/why-should-i-use-core-autocrlf-true-in-git

It probably depends also a lot on the user settings but maybe we still could safely add some files (like *.sh) to a gitattribute file?

PS: Github offers Discussions now. This issue would fit right into them.

[julie777]

I would like to implement the solution I proposed in the discussion

Implementing The Requirement

A good way to ensure the requirement is met, and to make sure it doesn't depend on precommit or local developer settings it to add the following two files to the project.

With this config-based approach, you can ensure that your line endings remain consistent in your codebase regardless of what operating systems or local Git settings your developers use since this file takes priority.

.gitattributes

* text=auto

Many code editors such as vscode will use this file if present to keep workspace file line endings correct.

.editor.config

root = true

[*]
end_of_line = lf

(If we decide to do this, I will gladly add the new files and make sure all existing files in the repo are normalized.)

[julie777]

Currently the only file in the repo with non LF endings is docs/gfx/logo.svg which has CRLF endings.
Given that all the other .svg files have LF line endings I don't imagine that changing logo.svg to LF endings will cause any problems.

[abravalheri]

Hi @julie777, please find bellow my attempt to summary of the presented arguments in favour of the change is (from what I could collect from the comments and links):

1. People have trouble with git's autocrlf=True configuration as it leaves the local repository in a different state than the remote. Using .gitattributes seem to offer a different (better) experience than autocrlf=True
2. .gitattributes travel with the repository so it is accessible for all contributors
3. .gitattributes is a git built-in and don't require other tools

If I am missing something here, please feel free to add.

These are my thoughts:

A. A direct comparison between git's autocrlf=True and the pre-commit hook does not seem to be fair. The pre-commit hook actually fixes the files in the local repository, so they there is no discrepancy between what you are submitting and what you have. Particularly for the pyscaffold repository, we go even further and force lf.

B. The arguments in 2 don't seem to be relevant for the pre-commit hook comparison, since it also travels with the repository.

C. PyScaffold own development process already requires a pre-commit installation anyway. There is a series of linting tasks performed that cannot be achieved using git only. With that in mind, I believe that is better to have a uniform approach for configuring all the linting/auto-fixing tasks and centralize as much as possible, instead of spreading it among more files.

D. There might be a point in adding .editorconfig, but only if it aggregates other factors and is not limited to the line endings. To assess that, it would be nice to know how much the out-of-the-box (or semi-out-of-the-box) editing experience from popular text editors (such as vscode/pycharm/vim/emacs/etc...) differ from what the current linting configuration enforces. So far my personal experience is that most of the time out-of-the-box editing experience is fine. If we add to that the fact that we use black, .editorconfig seem to loose relevance.

E. Changing the line endings in the logo should be fine.

[julie777]

@abravalheri good comments!

I thought the article I linked made a good case for .gitattributes vs pre-commit. I will add that although we are putting it in precommit in our .yaml file for projects that are created with putup, for a plain git user .gitattribute requires less understanding and you don't need to know about hooks.

Regarding A, the combination of .gitattribute and .editorconfig do maintain the repo and working dir in sync. Even without .editorconfig after a commit I believe the working dir will match what was commited (LF).

I also want to suggest another use of .gitattributes that we should consider, which means it won't be just for line endings.
Python 3 requires all source files to be utf-8 and also that they are not to be marked as such with a file header. We can use .gitattributes to tell git that python files are utf-8. This causes git to store the file as utf-8, remember that it is utf-8 and treat the file as text, for diff and other operations.

Regarding C, the use of precommit for actually running checks is very sensible and typical. I don't think maintaining line endings is really is this category since it is build it to git. Differing opinions 🙄

Regarding D, editorconfig should be added because it prevents problems. On the other hand black fixes them. I have actually been using vscode lately and have formatting hooked in so it gets fixed when I save. I think everything that can be done before the commit should be because, what the user last saw is what gets committed, and because the fewer times that precommit prevents the commit the better.

I personally like .editorconfig because I often jump from one project to another and often change editors. Trying to stay in sync with the way the project was set up can be difficult. (Of course not all the projects I look at were created with pysk.)

BTW, you mention that we use black. I can only find one mention of it in a comment in setup.cfg. This is a project I just created with putup. Can you please clarify "the fact that we use black"?

I do have another thought about precommit hooks. When I looked at the tests tox runs the are just the pytest ones. I would like to add all precommit hooks to the tests. I have always done it that way, thinking that they are part of the tests and if I as the developer do everything right then precommit should never fail. (This should probably be a separate issue.)

Similarly, I always had my team set up project so that precommit ran all tests that could be run in a dev environment. The ideas was to find out before going to CI, because it is easier to fix earlier. Often developers would run a subset of tests for speed of iterating on a code change. Then they ran all tests when they thought everything was ready. If everything passed then did the commit, which ran precommit just to make sure they didn't forget to run all tests first.

Aside, this really doesn't belong here, but in tox.ini [testenv] has

commands =
default: pytest -k "not system" {posargs}
system: pytest -k system {posargs}
all: pytest -n auto {posargs}

Is there a way to run a single command from a test env? I looked at tox doc and couldn't find a way.
If not why do we have separated commands and what appears to be running the same tests multiple times?