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

Code standards cleanup #378

Draft
wants to merge 62 commits into
base: master
Choose a base branch
from
Draft

Code standards cleanup #378

wants to merge 62 commits into from

Conversation

miken32
Copy link

@miken32 miken32 commented Jun 21, 2024

I wanted to do some work on this project for internal use but was struggling a bit with the code being in such inconsistent shape. (I say this without judgement, it's not surprising for a PHP codebase this old.) I'm cleaning it up to comply with PSR-12 and follow modern conventions as much as possible within the versioning restrictions the project has imposed.

To this point, I haven't intentionally done anything that would remove compatibility with PHP versions 5.4 to 8.0, and I would branch off before making any such changes. The end goal of course would be a properly namespaced codebase, likely using Composer, and compatible with current PHP versions.

It's a lot of changes and I don't expect it would be merged into the main codebase without a lot of consideration. This PR is just a heads up to let the maintainers know the fork is there, and solicit any opinions they have to share on the direction.

…ation MUST come after the visibility declaration
…omma, and there MUST be one space after each comma
…There MUST NOT be a space after the opening parenthesis
@LarsMichelsen
Copy link
Contributor

Thanks for willing to put effort into the project. I appreciate that people want to invest time to make NagVis better.

With such a huge amount of also mostly mechanical changes I think it's crucial not to let it lay around for too long and decide on the merges quickly. Ideally also merge them step by step to not create a too big chunk on the side.

If everything is mechanical and does not harm compatibility I don't see a strong reason against merging. However, I fear I don't have time to look into this myself.

@loocars maybe this is something you would like to do in between? I don't want to load more work on your shoulders with this, just pick it up if you feel it might actually help your other tasks.

@miken32
Copy link
Author

miken32 commented Jun 27, 2024

Indeed most of the changes are whitespace/stylistic (PSR-12) or in comments (all the typing stuff in PHPDoc blocks) so should not cause problems with compatibility, as long as 5.4 is your minimum supported version.

Don't take my word for it, but I believe these commits are the only ones that could potentially cause changes in behaviour: c876623, 7cd55a5, 935edd3, and 6b6a42a

With a few exceptions I've done the changes manually, using PHPStorm's code inspections or regex search/replace to find and fix problems. I did this so I could break it up into smaller chunks but even so I realize it's a lot of changes! Happy to answer any questions or make/undo changes as needed.

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.

None yet

2 participants