DX: Check trailing spaces in project files only #4957
Conversation
|
How to test: Use a differ of your choice to make the differences between the old and new behavior visible (here I took Reviewing the changes should show no files added by the new behavior (right side) but instead show false-positive files from the left side removed. |
|
@ktomk looks promising, can you point to The difference is that we lose colours: The first one is before and second after your changes - I don't know if we want to lose that, might be helpful for contributors to easily spot the trailing whitespaces. And if we don't need the colour I guess some formating from the line BTW have you tried with |
ktomk
force-pushed
the
use-git-instead-of-find
branch
from
3372df8
to
c28322d
Compare
|
@kubawerlos colors should be back with the last fixup, for git grep I'll take a look. |
|
@kubawerlos Yes, git-rep worked like a charm. No more execs of grep. Trailing whitespaces detected: - in CHANGELOG.md at line 14 > Changelog for v2.15.6
|
|
Hi, thanks for all the work! You think this should also be applied to https://github.com/PHP-CS-Fixer/trailing-whitespace-checker ? |
Previously the `check_trailing_spaces.sh` script was checking files that do not belong to the project, for example: - diverse `composer.lock` files - local development files like `.idea` or temporary files created In its' previous design, the list of files to check for trailing spaces is generated by `find` which is not aware about different files (not) in the project. To exclude non-project files, it worked with a larger blacklist of checking if a file is not in diverse paths, for example and most prominently: - -not -path "./.git/*" which is kind of contradictory as the project *is* managed by git, but that folder can have a totally different name. This blacklist is pretty verbose and also needs extra knowledge and care due to the nature of `find` as it operates on file-system level, not project level more specifically. Files in the project are managed by `git` and `git` knows which files belong into the project and which not. Git on ... - ... repository level contains the information which project files are ignored. for example the `composer.lock` files. - ... developer or system level also knows about which files are ignored. for example the `.idea` folder (see "global .gitignore") Change here is to replace `find` as file-system only utility with `git` itself, namely the `git-grep` command, for the operation to obtain the list of files to check. This includes the exec-call of grep, as gits' grep is a little more powerful, too [1] *and* is compatible in the output format for the post-processing with sed in the check-script. This allows to drop the manually crafted blacklist and to specifically mark paths/files not to be checked [2] which are (only) the test fixtures in: - tests/Fixtures/ The knowledge of git about the project itself and how the developer works next to the reduced list of files to check has the additional benefit of a cache and overall results in a much better performance. [1]: Fun with "git grep" <https://gitster.livejournal.com/27674.html> [2]: pathspec - gitglossary <https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec>
ktomk
force-pushed
the
use-git-instead-of-find
branch
from
dd7c53f
to
865d1ed
Compare
|
Just squashed after review.
I don't have it in use, no idea. The port is not straight forward at least for me as it needs also support for Subversion for which I currently don't have a system prepared for. Feel free to open an issue there if you need it there, too, then we can discuss the details of a port there. Just ping me. |
|
Thank you @ktomk. |
(minor change despite longer description)
Previously the
check_trailing_spaces.shscript was checking files that do not belong to the project, for example:composer.lockfiles.ideaor temporary files createdIn its' previous design, the list of files to check for trailing spaces is generated by
findwhich is not aware about different files (not) in the project. To exclude non-project files, it worked with a largerblacklist of checking if a file is not in diverse paths, for example and most prominently:
which is kind of contradictory as the project is managed by git, but that folder can have a totally different name.
This blacklist is pretty verbose and also needs extra knowledge and care due to the nature of
findas it operates on file-system level, not project level.Files in the project are managed by
gitandgitknows which files belong into the project and which not. Git on ...composer.lockfiles..ideafolder (see "global .gitignore")Change here is to replace
findas file-system only utility withgititself, namely thegit-ls-filescommand, for the operation to obtain the list of files to check. thefindpackage (findutils) is not replaced as thexargsutility from it is in use.This allows to drop the manually crafted blacklist and to specifically mark paths/files not to be checked which are (only) the test fixtures in:
The knowledge of git about the project itself and how the developer works next to the reduced list of files to check has the additional benefit of a cache and overall results in a much better performance.