search

php-parallel-lint / PHP-Parallel-Lint

This tool check syntax of PHP files faster than serial check with fancier output.
Other
282 stars 21 forks source link

Split files for PSR-4 compliance #45

Closed roelofr closed 2 years ago

roelofr commented 3 years ago

There were many classes combined into single files, which isn't recommended and makes maintaining hard.

For example; Did you know that \JakubOnderka\PhpParallelLint\ArrayIterator was actually located in src/Settings.php. Not where you'd expect it to be.

This PR moves all these classes to their own files, and configures Composer to use a PSR-4 autoloader instead of a classmap.

Closes #25 Closes #26

roelofr commented 3 years ago

Yeah, when I was refactoring the code I noticed that my src/ folder was growing at a rapid pace, with Exceptions not being namespaced.

I dabbled about doing that too, but being a first-time contributor to this project, that seemed a bridge too far to cross on the first PR.

jrfnl commented 3 years ago

@grogy Could you please pitch in to say whether you agree with the suggestions above ?

Would be great if we could get this PR moving again to eventually get it merged.

roelofr commented 3 years ago

Okay, the codebase is split up into some folders, and I ran phpstan to make sure all links were correct.

Unit tests still fail locally (something about JsonSerializable not being found, while ext-json is active).

grogy commented 3 years ago

I readed changes commit to commit and it makes sense. I see a problem that I missed that PR and codebase is far away.

For resolve and merge I can rebase and solve it locally and force push it, or you can rebase it onto current master?

roelofr commented 3 years ago

I've rebased the commits, the only big change seemed to have been the GitLabOutput.

A difference since the previous base is here

roelofr commented 3 years ago

I noticed the tests were failing on a missing polyfill, so I've added that back in.

jrfnl commented 3 years ago

I noticed the tests were failing on a missing polyfill, so I've added that back in.

Please load that file via a require_once in the bootstrap instead of letting Composer autoload it. See the dicussion about this here: https://github.com/php-parallel-lint/PHP-Parallel-Lint/pull/51#discussion_r593924479

roelofr commented 3 years ago

@jrfnl I tried removing it, but that doesn't work in combination with PHP 5.3, so I re-added it as autoload-dev, which should prevent it from loading in projects requiring this package.

jrfnl commented 3 years ago

I tried removing it, but that doesn't work in combination with PHP 5.3, so I re-added it as autoload-dev, which should prevent it from loading in projects requiring this package.

Using autoload-dev feels wrong as the polyfill is needed for non-dev when the JSON extension is not available. Hmm.... I don't have to time to look into this further at the moment, but I do think we need to find another solution.

roelofr commented 3 years ago

I've removed the commit for now, but as said, it won't work.

The assumption made in the comment mentioned by @jrfnl is, in my opinion, out of scope.

This would prevent potential issues with other dev tools which may use an interface_exists() check on JsonSerializable which would now suddenly start passing for them due to the file being automatically loaded (even when this tool is not being run).

If devtools use the existence on JsonSerializable to check if json is installed, they won't work on php 5.3 to begin with.

Wrapping the polyfill in a version_compare(PHP_VERSION, '5.3', '<=') should therefore suffice, and we can still include the polyfill via composer.

roelofr commented 3 years ago

@jrfnl Did you manage to think about how we're resolving the polyfill issue? I think including it in composer.json is okay, as it's what we're doing in our current latest version... :thinking:

jrfnl commented 2 years ago

I've taken the liberty to rebase this PR and add an extra commit changing the namespace prefix to PHP_Parallel_Lint\PhpParallelLint to be in line with the other repos in this organisation.

Will have a look at the PHP 5.3 failure now.

jrfnl commented 2 years ago

(sorry for the long wait, but 🤞🏻 we'll get this merged now soon)

jrfnl commented 2 years ago

I've added an extra commit to sort out the test run on PHP 5.3.

As the polyfilled class will no longer be loaded via the Composer autoload file (as it doesn't have the expected namespace), the file needs to be required in the tests.

When the tests get refactored to PHPUnit, these requires will move to a test bootstrap file instead.

glensc commented 2 years ago

PHP_Parallel_Lint as namespace is terrible idea. It likely conflicts with psr-0 that composer has for legacy pear packages. or create some other unknown issues. avoid that. just remove underscores.

jrfnl commented 2 years ago

PHP_Parallel_Lint as namespace is terrible idea. It likely conflicts with psr-0 that composer has for legacy pear packages. or create some other unknown issues. avoid that. just remove underscores.

I hear you, but a) conflicts will only happen if someone would be using the same namespace and b) the PHP_Parallel_Lint prefix has already been implemented in Console Color and Console Highlighter - using a different prefix here would be terribly inconsistent.

roelofr commented 2 years ago

Personally, it stings that the PR I made is now made so ugly with this new namespace change (which I think is really ugly, and a step backwards from no namespaces), but it's been nearly a year and I can't really care anymore.

Good luck with this PR and the project, and thanks for looking into this after a year

jrfnl commented 2 years ago

Personally, it stings that the PR I made is now made so ugly with this new namespace change (which I think is really ugly, and a step backwards from no namespaces), but it's been nearly a year and I can't really care anymore.

Good luck with this PR and the project, and thanks for looking into this after a year

@roelofr I'm sorry you feel this way and I empathize with your frustration about how long the PR has been open. Has happened to me too often as well. The joys of open source....

jrfnl commented 2 years ago

Rebased without changes after the merge of #92 to get past the imaginary merge conflict.

grogy commented 2 years ago

@roelofr thanks for the merge request. I know that long time to merge is frustrating. I am sorry to you.

Today I call with @jrfnl and we discuss about namespaces. We concur that same namespace in whole organization is best way. I am remember that is break change. This change will be released in 2.x version.

Merge request looks good and I does not have now some suggest, so I am merged it. Thanks to all in discussion, @roelofr for original MR and @jrfnl for rebase.