Skip to content

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jun 2, 2020

When you're using allowOnly = first, you dont expect 'Boolean operators between conditions must be at the beginning or end of the line, but not both' as error message because end of line is not allowed.

So I improved it.

@gsherwood gsherwood added this to the 3.5.6 milestone Jun 11, 2020
@gsherwood gsherwood modified the milestones: 3.5.6, 3.6.0 Jun 22, 2020
@jrfnl
Copy link
Contributor

jrfnl commented Sep 28, 2020

I just ran into this again and started to create a PR only to realize one is already open.

@gsherwood As this is only a message change, is there a particular reason for this change to be milestoned to 3.6.0, not 3.5.7 ?

The existing tests cover the change, but just to make the case for this change, here is a test scenario which illustrates the problem this PR solves:

When the "allowOnly" option is enabled, the error message for a condition with only two parts is unclear and confusing.

For example, given this ruleset:

<?xml version="1.0"?>
<ruleset>
    <rule ref="PSR12.ControlStructures.BooleanOperatorPlacement">
        <properties>
            <property name="allowOnly" value="first"/>
        </properties>
    </rule>
</ruleset>

... run against the below code:

if (
    $conditionA === true ||
    $conditionB === true
) {
    // Do something.
}

Would result in the following error message:

 [x] Boolean operators between conditions must be at the beginning or end of the line, but not both
     (PSR12.ControlStructures.BooleanOperatorPlacement.FoundMixed)

... which would presume that the end-user knows that the ruleset demands for the boolean operator to be at the beginning of the line as otherwise the error doesn't make any sense.

@gsherwood gsherwood modified the milestones: 3.6.0, 3.5.7 Sep 29, 2020
@gsherwood
Copy link
Member

As this is only a message change, is there a particular reason for this change to be milestoned to 3.6.0, not 3.5.7 ?

It was done at a time when I was trying to clear 3.5.7 and push through just a few bug fixes. But it's turned into a much bigger bug fix release, so makes sense to include this change again. I've adjusted the milestone for it.

gsherwood added a commit that referenced this pull request Sep 29, 2020
@gsherwood
Copy link
Member

Thanks a lot for this message change. It has been merged in, but I was doing a rebase and the commit hashes got changed over.

@gsherwood gsherwood closed this Sep 29, 2020
@VincentLanglet VincentLanglet deleted the fixErrorMessage branch September 29, 2020 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants