Add a new tidy.php script to fix whitespace and EOL issues #3505
Conversation
|
I wonder if there's some need to add a hook to check for whitespaces before commit changes. "Trim trailing whitespaces" is a common PR: #2995 |
|
Comment on behalf of carusogabriel at php.net: Labelling |
petk
force-pushed
the
patch-trailing-whitespaces-1
branch
3 times, most recently
from
a44afb8 to
3f812a3
Compare
|
Hmm, if we remove trailing whitespace from master only, there may be merge conflicts in the future. If we remove it from 7.1+, there still may be future merge conflicts, since security fixes are still applied to 5.6+. Removing trailing whitespace from 5.6 is certainly a no-go, since the branch is locked for security fixes only. Therefore we cannot avoid potential merge conflicts, if we ever remove the trailing whitespace. However, considering that there have been many merge conflicts between 5.6 and 7.0 anyway, I can easily image that this will be similar with 8.0, so perhaps it would be best to remove the trailing whitespace for this version only. The same considerations apply to the file endings (PR #3516). If in doubt, this issue should probably be discussed on internals@. |
|
Yes, definitely it's a bit big change with several hundred files... What I was thinking here also a bit is the supported versions timeline where PHP 5.6 and 7.0 might cause merge conflicts only for few months if that applies... And PHP 7.1 will be still supported for a whole year. |
|
Before we move forward with any of the whitespace cleanup PRs currently in the pipeline, I'd like to make sure that we can replicate the results in an automated manner in the future, as the current situation does not seem to be particularly sustainable. Specifically, I'd like this PR to include a This can also serve as a pre-commit hook or as a CI check in the future, but for now I'm mainly interested in a solution that we can rerun every couple months to maintain the status quo. |
|
It might be also an idea to evaluate, whether some good tool can be used for this purposes. Just to call some - Thanks. |
petk
force-pushed
the
patch-trailing-whitespaces-1
branch
from
3f812a3 to
be7a679
Compare
|
I've added an initial Additional dedicated tool(s) such as ClangFormat would be ideal for another case - formatting C code. PHP src has C, PHP, text, shell, and other files that wouldn't get this cleaned from what I'm testing in such customized ways, I think... But I'll recheck the ClangFormat a bit more. |
We can make usage of Travis's Build Stages (https://docs.travis-ci.com/user/build-stages) to add a "Whitespace Check" stage after running tests to ensure no new whitespaces were inserted. |
There was a problem hiding this comment.
Talking a bit about the work done in #3507 trimming trailing whitespaces in tests, I used the logic that we have in run-tests.php to split the sections and I've trimmed the TEST, FILE, SKIPIF, etc, that didn't affect the test. I had some issues with encoding, but I believe we can update the tidy script to also support our tests :)
|
@petk yeah, those tools mentioned are mainly for C/C++. Thanks. |
petk
force-pushed
the
patch-trailing-whitespaces-1
branch
4 times, most recently
from
c1e7dd0 to
9a7b1c5
Compare
|
@carusogabriel Otherwise, the Then, the fixing procedure will be very trivial: cd php-src
git pull --rebase
git checkout PHP-7.1
scripts/dev/tidy.php . --{options-yet-to-be-determined}
# check the changes
git add .
git commit -m "TL;DR of the changes"
git checkout PHP-7.2
git merge --no-ff --log -s ours PHP-7.1
scripts/dev/tidy.php . --{options-yet-to-be-determined}
# check the changes
git add .
git commit -m "TL;DR of the changes"
git checkout PHP-7.3
git merge --no-ff --log -s ours PHP-7.2
scripts/dev/tidy.php . --{options-yet-to-be-determined}
# check changes
git add .
git commit -m "TL;DR of the changes"
git checkout master
git merge --no-ff --log -s ours PHP-7.3
scripts/dev/tidy.php . --{options-yet-to-be-determined}
# check changes
git add .
git commit -m "TL;DR of the changes"
git push origin PHP-7.1 PHP-7.2 PHP-7.3 masterOne issue is that running |
- in_array strict comparison - remove trimming leading whitespace (trailing whitespace is self explanatory enough and fixes those also) - add peak memory consumption info in results
- --all option removed - --clear-space-before-tab enabled - various fixes with tidying phpt files
Instead of --no-backup option a better usability is to move this to prompt step and make -y and -q options without backups.
|
So far the following tidying fixes have been applied in PHP-7.1, 7.2, 7.3 and master branches:
Edit 1: Done step 4 - trailing whitespace in *.phpt |
Due to two edge cases, --trim-trailing-newlines is disabled for --FILE-- sections in phpt files.
|
Hello, considering the last 6 months of code style changes, the people involved in this project, their preferred coding styles, their reactions to code styles, and the level of giving importance to code style consistencies, I think PHP repo/org is not ready for automated code style checks. I think this can be closed and future adjustments done manually per file basis like now... Script will be maintained elsewhere out of PHP. Basically, also the main goal has been partially reached - majority of the minor CS cleanups for the PHP-7.1+ has been done (at least those most annoying ones)... Basis for something is formed but a lot more should be done in the future which I'm not sure everyone has time nor will for this. Hopefully more in the future. Also another reason for closing this: 1000-2000 lines of code in a single file is from my point of view way too much to properly comprehend and process. And such script requires a bit more than just a single file procedural coding. This script alone requires a separate repository and tests. Similar conclusions can be reached for integrating some dedicated tool such as clang format, php-cs-fixer etc. Thanks everyone for their feedbacks. |
Hello, this patch trims trailing whitespaces in source code files for the master branch:
excluding the following locations:
And excluding files generated using internal scripts:
Note, that some editors automatically trim trailing whitespaces so this is kind of for convenience of editing code also.
Once there were mentioned also merge conflicts in #3091 to trim these in PHP-7.1 and up also. Would it be ok to make separate pull requests for PHP-7.1, PHP-7.2, and PHP-7.3 so merging patches won't cause merge conflicts due to whitespaces in the future?
For easier viewing of the changes done here there is also
?w=1GitHub URL parameter, which doesn't show whitespace changes...