search

NagVis / nagvis

Visualization addon for your open source monitoring core
http://nagvis.org/
GNU General Public License v2.0
113 stars 73 forks source link

Code standards cleanup #378

Open miken32 opened 2 weeks ago

miken32 commented 2 weeks ago

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.

LarsMichelsen commented 1 week ago

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 commented 1 week ago

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: c8766239, 7cd55a5a, 935edd30, and 6b6a42aa

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.