Introduction of the concept of Severity in order to distinguish errors from warnings.

[MisterIcy]

It seems that SARB is only aware of the concept of Error; it doesn't know anything about warnings or what it should do with them.

In its most simplistic form, I could say that a warning (or a list of warnings) should be shown, but the tool should return success instead of failure, in case the baseline contains only warnings.

Since this is a fairly complex feature to implement and extra caution must be taken in order not to introduce any BC-breaking changes, I thought it would be better to ask, is submitting a Pull Request that introduces the concept of severity to SARB, has any meaning.

Thanks in advance, Alex.

[DaveLiddament]

Hey Alex.

Thanks for the suggestion. I think warnings could be introduced into SARB. It'd be great to learn more about the use-cases that would be beneficial to you, to make sure we make the right feature.

Have you any example tools that you create baselines for that have both errors and warnings?

In terms of BC breaks....

1. Shortly after PHP 8.2 is released I'm going to make a release that drops support for PHP 7.x, so there is an opportunity to drop in more BC breaks in that release.
2. Given this SARB is a tool, rather than a library, the only thing that we really have to be careful about is changes to the generated baseline file. If the baseline files generated by SARB can operate both with and without warnings, then, for the majority of users, this would not be a BC break.

If you let me know a bit more about your specific use cases, we can do a bit more discussion here for a workable solution and then start creating the feature.

Thanks once again, for you interest in SARB.

[MisterIcy]

Hey there Dave,

I am working in a really old codebase and decided to switch to PSR-12. I ran phpcs, it found a ton of stuff that needs fixing, I generated a baseline via SARB and everything was awesome.

Since phpcs does know the concept of warning, I instructed it to display warnings when the name of a class's member is prefixed with underscore (which used to declare its visibility, take this for example).

class Foo {
    // What I should have:
    protected function bar(string $baz) {}
    // What I really have:
    protected function _bar($baz) {}
}

I can't change the name of protected methods, since I don't want to break BC for the time being. But I'd like to add parameter / return types and fix various other issues.

class Foo {
    // PSR-12 Power!
    protected function _bar(string $baz): array
    {
    }
}

After doing this change, PHPCS will display a warning, since I configured it to display warnings when it finds a leading underscore in the name of a class member. However, since the code was changed, without regenerating the baseline, SARB thinks that this is a new issue (and this is the desired and expected behavior) and reports it. However, lacking the concept of warning and what it should do with them, it reports an error and exits with 1.

At the moment, my only choice is to regenerate the baseline each time I make such a change.

My idea was to introduce the concept of Severity to SARB. PHPCS would report both errors and warnings depending on its configuration and SARB would be aware not only of the Type of the issue but also of the Severity of the issue.

Reporting can benefit from the concept of Severity, some things are more severe than others. Still, we want to be aware of those in order to prevent their introduction and, eventually, fix them.

Also it would be really helpful if SARB gives us the choice to report all issues but fail (exit code != 0) only on a certain condition. For example, it should fail only if it encounters issues of error severity, but it will report success if it encounters only warnings.

However, this is a very challenging task due to SARB's amazing design. I can open a draft PR if you want to have a look at what I have in my mind.

Thanks for your time :)

[DaveLiddament]

Hi @MisterIcy

Thanks for that helpful description of what you want. I think we can achieve that with SARB, and can probably do it in a way that it is a non breaking change.

I've seen your PR (https://github.com/MisterIcy/sarb/pull/1). Thanks for this, it looks a great start. There are a few bits I'd like to change. Do you mind me basing code from this PR? We can chat about the changes before I merge it into SARB, to check you're happy with them.

Thanks

Dave

[MisterIcy]

Hey @DaveLiddament,

Feel free to use anything from the PR, it's just a prototype of the idea, since I'm terrible at designing things.

Thanks again for your interest in this functionality.

Alex

[DaveLiddament]

@MisterIcy Sorry for the delay in getting this out. I've got something that is close to what you've asked for.

Both errors and warnings are reported. However you can suppress warnings using the --ignore-warnings flag.

PR is #124

[DaveLiddament]

Closed by PR #124