-
Couldn't load subscription status.
- Fork 544
Add --only-remove-errors analyse command option #3847
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
Add --only-remove-errors analyse command option #3847
Conversation
A problem I just realized while creating the PR: ignoreError configurations done outside of the baseline would now be put in the baseline as well, which we probably want to avoid.
Also noticed ci failing because a bigger baseline is generated, looking into it
This needs a different approach.
- Remove all the changes around config loading etc. because they need to stay the same, otherwise we're risking unintended side effects.
- BaselineNeonErrorFormatter and BaselinePhpErrorFormatter need to be aware of the new CLI option user passed in, and also about the baseline file name currently being overwritten.
- BaselineNeonErrorFormatter already does that to a degree, to preserve the number of newlines at the end of the file.
- Based on the current file and the fact that the user only wants to remove errors, both BaselinePhpErrorFormatter and BaselineNeonErrorFormatter could implement this logic, without changing anything else in PHPStan.
hey @ondrejmirtes thank you for the feedback!
That's the approach im currently trying as well.
Im basically going through IgnoredErrorHelperResult again with only the ignores from the baseline.
One Problem I am having is how to best load only the baseline config. I'm currently loading it in the CommandHelper, because the Loader is available at that point. I'm thinking maybe we could add the loader to the DI Container to use that in the generateBaseline method, I am not sure yet though.
Will report back with a new version of the chnges.
I don't think we need to touch IgnoredErrorHelperResult or CommandHelper.
AnalyseCommand should just pass the information about the file and the option to the baseline formatters. Nothing else should be touched.
Tried a new approach. I did not add the code the baseline formatters but to the generateBaseline function of AnalyseCommand as to not duplicate the logic.
To avoid side effects, the code is only run when the new cli option is passed.
I'm not quite happy with the structure of the tests but found them to be quite tricky to get right because the File names need to stay the same while changing the content. Needed some ignores, which is probably not ideal.
Looking forward to hear your feedback on the code changes.
This pull request has been marked as ready for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The option should be renamed. Something like
--generate-baseline --only-remove-errorsmakes more sense to me. - The actual logic shouldn't be implemented inside AnalyseCommand, but as a separate class that accepts two arrays: baselinedErrors, currentAnalysisErrors and returns errors that should be put into the baseline. That way we can easily unit test it.
This reverts commit 389b6d1.
causes failure in pipeline where less text is rendered on one row
67d4474 to
0f27eea
Compare
Thanks for the review.
Extracted the new logic into BaselineIgnoredErrorHelper, which I added to the config.
Deleted the old integration tests to replace them with unit tests.
Following the discussion I created, this is my attempt at adding a command line option for ignoring new errors to trim down the baseline file.
This enables the following workflow:
--generate-baseline --ignore-new-errorsto remove the fixed ignored errors from the baselineThis way, the unmatched ignored errors are removed from the baseline and from the analysis output, allowing me to focus on the new ones.
As a first step, I wanted to open this PR to validate the idea, I'm not 100% confident in the changes I made and their consequences.
The biggest change here is, that I changed configuration loading in such a way, that the baseline configuration would be loaded, even when generating the baseline. That means, that we need to remove the ignoreErrors configuration in the "IgnoredErrorHelperResult". I hope this is the only place where that configuration change effects the code, the tests seem to support it.
There is now more "argument drilling" than before and the Fixer command needs to pass the baseline and ignoreNewErrors arguments, which I have set to false on that part for now, because I don't think the fixer uses baselines.
I'd be happy to hear some early feedback on this.