Open fredden opened 5 months ago
Thanks for opening this issue @fredden. You already know my opinion: this is more complicated than you initially thought.
If I look at the table above, I still see logic logic errors.
If the caching feature is active, the cache at the end of a run should always be complete and correct, so in my opinion, your entries in the "Only fixable errors" and "Fixable and unfixable errors" rows for phpcbf
are incorrect.
In the mean time, let's close #481.
@jrfnl let's discuss this on our next call. I am interested to learn about these logic errors that you hinted at. I think that the implementation in #481 matches the table above.
Wouldn't it be possible to start small? Just skip the files that are already cached and with no errors if unmodified since last phpcs run. Even if it does not update the cache and re-runs for fixed files before phpcs is ran again, it would still make fixing few files in codebases with 1000+ files significantly faster and not break anything. And updating the cache would be as simple as running phpcs again. It would be far from perfect but from my (possible naive) point of view it would do 90% of the results for 10% of the work, with very little (possibly none?) risk involved.
In my call with @jrfnl, we determined that there are not actually any logic errors, but instead the language used was confusing. I will review the words/language to better communicate the intent.
Just came across old repo issue https://github.com/squizlabs/PHP_CodeSniffer/issues/3781 and I think that needs to be looked into in relation to this ticket as it gives the impression that - at least in some cases - the cache is already being taken into account for PHPCBF....
In my call with @jrfnl, we determined that there are not actually any logic errors, but instead the language used was confusing. I will review the words/language to better communicate the intent.
I have replaced the table with a flow-chart, and written more words in the description.
Just came across old repo issue squizlabs/PHP_CodeSniffer#3781 and I think that needs to be looked into in relation to this ticket as it gives the impression that - at least in some cases - the cache is already being taken into account for PHPCBF....
From what I can tell, that bug seems independent of this issue / feature request. I have put a comment there with my findings.
would end-to-end tests be sufficient to get this moving forward or do we also need unit tests for the caching related stuff?
@staabm I have a feeling a combination of both will be needed. With end-to-end, we'd probably need too many tests to cover it properly, with a combination, the effort needed to get the tests in place will probably be lower.
Is your feature request related to a problem?
Yes and no. When using
phpcs --cache
, subsequent runs are fast. When usingphpcbf --cache
, every run takes the same amount of time as the first.Describe the solution you'd like
When using
phpcbf --cache
, subsequent runs should be faster - as they already are withphpcs --cache
. Also, when using bothphpcs
andphpcbf
, the cache generated by one should be usable by the other.phpcbf
should be able to use the cache to determine if the sniffs need to be run or not on a particular file. When the cache is valid for a file, and there are no fixable errors reported, then the file can be skipped. When the cache is invalid, or there are fixable errors reported, then the file needs to be run through the sniffs as normal.Internally when
phpcbf
runs, there is actually a run of (the equivalent of)phpcs
first, and if there are any fixable errors reported then the fixer is enabled and run through the file. When the fixer is enabled, the cache should be temporarily disabled to avoid any negative performance overhead as it runs the sniffs up to 50 times.At the end of a run of
phpcbf
, the cache does not need to be in a perfect state. If there were no changes made, then the cache should still be valid for the files it was valid for to start with. If any changes were made, then the state of the cache is undefined. The next time the cache is read, it will be validated as normal.phpcs
This is the existing behaviour for
phpcs --cache
.phpcbf
This is the proposed behaviour for
phpcbf --cache
.Additional context (optional)
Given the low test coverage of the areas in question here, making changes to this functionality is risky. Therefore this feature is likely blocked by https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/146 and/or https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/147
The proposed changes have been implemented in https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/481