Skip to content

Tidy infrastructure: Reindent PHP/PHPT #5074

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

Closed
wants to merge 2 commits into from
Closed

Tidy infrastructure: Reindent PHP/PHPT #5074

wants to merge 2 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Jan 10, 2020

Add scripts/dev/tidy.php to automatically enforce our formatting / indentation rules.

In particular, this removes trailing whitespace and reindents all PHP files and PHPT tests to spaces. Tests can be marked as --WHITESPACE_SENSITIVE-- if the whitespace is actually relevant for the test.

Reindentation is not performed for C yet, as it sometimes has mixed results.

@kocsismate
Copy link
Member

kocsismate commented Jan 10, 2020

Yes! I was also thinking about something like this for only .phpt files. Indentation was my frustration too (space vs tabs + 2 vs 4 spaces), however I could also imagine having PSR-12 formatting where it is applicable (ok, it's wishful thinking at this point).

@nikic nikic changed the title Tidy infrastructure (1): Reindent C files Tidy infrastructure: Reindent PHP/PHPT Jan 10, 2020
@nikic
Copy link
Member Author

nikic commented Jan 10, 2020

I've decided to avoid the C reindentation for now (as the results are somewhat mixed -- because tabs). Instead this now implements the actually important part, which is PHP and PHPT reindentation.

There's a new --WHITESPACE_SENSITIVE-- marker for tests that should be left alone, because they specifically test whitespace.

@nikic
Copy link
Member Author

nikic commented Jan 10, 2020

Quite a few failures due to tests that have trailing whitespace that is also part of the output :( Those will all have to be fixed first.

@kocsismate
Copy link
Member

Can't they all be just marked --WHITESPACE_SENSITIVE-- for now? Or fixing meant exactly this?

@nikic
Copy link
Member Author

nikic commented Jan 10, 2020

I think it would be better to avoid that marker unless the test really tests something about whitespace (e.g. flexible heredoc parsing).

Fixing is more along the lines of e748a5a...

php-pulls pushed a commit that referenced this pull request Jan 31, 2020
@kocsismate
Copy link
Member

I fixed as many indentation/trailing whitespace issues as I could in 2015c7a, so can you please rebase to see how many failing tests are left? I can deal with them as well.

@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

@kocsismate Done

@kocsismate
Copy link
Member

Is it possible that it's not the latest master? I found some failures where the expected output is the same as in current master (e.g. ext/dom/tests/DOMNode_cloneNode_basic.phpt). 🤔

@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

@kocsismate This currently targets the 7.4 branch. Could of course only do it for master, but might cause merge issues?

@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

It might make sense to apply the .php formatting to 7.4, but the .phpt formatting only to master. We do most test changes on master only, and the master tests have diverged quite a bit from 7.4 anyway, for various reasons...

@kocsismate
Copy link
Member

It might make sense to apply the .php formatting to 7.4, but the .phpt formatting only to master.

Yeah, it sounds reasonable. Although, I already tried out what happens when I cherry-pick the changes I introduced in master to the 7.4 branch. The result doesn't seem very bad, so I am ok to redo the fixes there as well if it's beneficial at the long-term.

@nikic nikic mentioned this pull request Feb 3, 2020
@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

I've opened #5145 for a more reduced 7.4 variant.

kocsismate added a commit to kocsismate/php-src that referenced this pull request Feb 3, 2020
kocsismate added a commit to kocsismate/php-src that referenced this pull request Feb 3, 2020
@nikic nikic changed the base branch from PHP-7.4 to master February 3, 2020 12:45
@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

Now targeting master.

@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

Travis failures:

highlight_file() tests [ext/standard/tests/strings/highlight_file.phpt]
Test str_replace() function [ext/standard/tests/strings/str_replace_variation2.phpt]
Test vfprintf() function : usage variations - unsigned formats with signed and other types of values [ext/standard/tests/strings/vfprintf_variation16_64bit.phpt]
Test vprintf() function : usage variations - char formats with non-char values [ext/standard/tests/strings/vprintf_variation10.phpt]
Test vprintf() function : usage variations - octal formats with non-octal values [ext/standard/tests/strings/vprintf_variation12_64bit.phpt]
Test vprintf() function : usage variations - hexa formats with non-hexa values [ext/standard/tests/strings/vprintf_variation14_64bit.phpt]
Test vprintf() function : usage variations - unsigned formats with signed and other types of values [ext/standard/tests/strings/vprintf_variation16_64bit.phpt]
Test vprintf() function : usage variations - scientific formats with non-scientific values [ext/standard/tests/strings/vprintf_variation18.phpt]
Test vsprintf() function : usage variations - octal formats with non-octal values [ext/standard/tests/strings/vsprintf_variation12_64bit.phpt]
Test vsprintf() function : usage variations - hexa formats with non-hexa values [ext/standard/tests/strings/vsprintf_variation14_64bit.phpt]
Test vprintf() function : usage variations - float formats with non-float values [ext/standard/tests/strings/vprintf_variation6.phpt]
Test vprintf() function : usage variations - string formats with non-string values [ext/standard/tests/strings/vprintf_variation8.phpt]

@kocsismate
Copy link
Member

@nikic Yeah, I saw. I've been fixing them :)

@kocsismate
Copy link
Member

... and done in 0253a23

@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

@kocsismate
Copy link
Member

Noted. Fix is on the way.

@kocsismate
Copy link
Member

9942f45 hopefully fixes the issues... (this time I restricted myself not to change the indentation of format specifiers in order to reduce the scope).

@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

Merged as d2cb200 and f8d7958.

@nikic nikic closed this Feb 3, 2020
@nikic
Copy link
Member Author

nikic commented Feb 3, 2020

@kocsismate Thanks a lot for sorting out all the formatting issues!

@kocsismate
Copy link
Member

kocsismate commented Feb 3, 2020

You're welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants