Skip to content
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

[Coding Style] Enable rule PSR12.Files.FileHeader + unify file headers #22132

Merged
merged 8 commits into from
Apr 20, 2024

Conversation

michalkleiner
Copy link
Contributor

@michalkleiner michalkleiner commented Apr 17, 2024

Description:

While applying the CS rule to fix spacing around file headers I noticed there are some inconsistencies, so I've also bulk unified them across the codebase since we were already updating over 2000 files just for the CS rule.

We will add the commit to the .git-blame-ignore-revs file as well so it's fitting to update all the headers at once as well.

I reviewed all the files manually before committing them using local git UI (Fork) and I can confirm the change is safe and doesn't alter any functionality.

Main changes:

  • new line additions around file headers
  • changing http to https in file headers
  • tweaking spacing in file headers
  • rebuilding dist files as the file headers are included in the dist files as well

Review

@michalkleiner michalkleiner added Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review Technical debt Issues the will help to reduce technical debt labels Apr 17, 2024
@michalkleiner michalkleiner added this to the 5.1.0 milestone Apr 17, 2024
@sgiehl
Copy link
Member

sgiehl commented Apr 19, 2024

This one actually isn't reviewable at all. GitHub does not even support to show so many changes.
If you want to have that reviewed, I guess you might need to split up the PR like I did it with this one: #21926

@michalkleiner
Copy link
Contributor Author

michalkleiner commented Apr 19, 2024

Thanks for the feedback Stefan.
As I expected that, it was why I've written in the description that I reviewed the files locally to try and alleviate that concern. I've actually looked at all the files one by one as it's quite easy to see relatively quickly that only one or two lines were added at the same place in each file, so it was basically just hitting enter to stage the files one by one and only very small handful of files needed manual intervention. I also committed the changes in separate commits to help with that. Yes, we can split the PR into 10 smaller ones as you did before, I can do that if that makes it easier to get that through, but I'm not sure it's time well spent. Alternatively, it's also possible to view the change in local git gui. Let me know what you prefer.

@sgiehl
Copy link
Member

sgiehl commented Apr 20, 2024

@michalkleiner For future PRs of that size it would be better to have smaller chunks. For that one I guess we simply leave it as is. I only had a look through the changes at random. As it's no easily possible to look through such big commits at once without loosing focus 🙈
Btw. For having unified headers across all files it could maybe be useful to create a custom sniff somewhen in the future.

@sgiehl sgiehl merged commit 9a3ef94 into 5.x-dev Apr 20, 2024
21 of 25 checks passed
@sgiehl sgiehl deleted the psr12_fileheader branch April 20, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. Technical debt Issues the will help to reduce technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants