PHPCSStandards / PHPCSExtra

A collection of code standards for use with PHP_CodeSniffer
GNU Lesser General Public License v3.0
90 stars 8 forks source link

Ignore specific keys for Universal.Arrays.MixedArrayKeyTypes and Universal.Arrays.MixedKeyedUnkeyedArray #278

Closed kkmuffme closed 11 months ago

kkmuffme commented 11 months ago

Is your feature request related to a problem?

Some libraries expect some keys to be string and others to not be set, and since it's an external library I cannot change how it accepts the array.

array(
'relation' => 'AND',
array( 'key' => 'foo' ),
array( 'key' => 'bar' ),
)

While I have only 1 explicitly keyed element, I might have 20 non-keyed, which means I would have to add 20 phpcs:ignore or phpcs:disable a lot of times.

Describe the solution you'd like

An "allow" (or "ignore") property list for both rules.

    <rule ref="Universal.Arrays.MixedArrayKeyTypes">
        <properties>
            <property name="ignore" type="array">
                <element value="relation"/>
            </property>
        </properties>
    </rule>
    <rule ref="Universal.Arrays.MixedKeyedUnkeyedArray">
        <properties>
            <property name="ignore" type="array">
                <element value="relation"/>
            </property>
        </properties>
    </rule>

Additional context (optional)

jrfnl commented 11 months ago

Some libraries expect some keys to be string and others to not be set, and since it's an external library I cannot change how it accepts the array.

Any array item without an explicit key, automatically gets assigned an integer key. You can fix the issue by using explicit integer keys yourself, like so:

array(
'relation' => 'AND',
0 => array( 'key' => 'foo' ),
1 => array( 'key' => 'bar' ),
)

No need for disables/ignores. Either fix the issue or don't use this sniff.

kkmuffme commented 11 months ago

Adding the default numeric keys just makes it harder to maintain. Some months later, I add an array at position 2 - then I have to manually change all following 20 elements too. This makes it a pain to work with and also the git diffs are less clear.

jrfnl commented 11 months ago

@kkmuffme Again, in that case, this is just not a suitable sniff for your usecase.

kkmuffme commented 11 months ago

I know I can phpcs:disable, but for 90% of my code this sniff works perfectly fine and makes sense. But in 10% I have some external libs, that have the above issue - I can of course phpcs:disable each and every time, but an ignore option would be much better, as I'd only have to set this in the config once.

jrfnl commented 11 months ago

Another way to still use the sniff, but avoid this particular issue is as follows:

$array = array(
array( 'key' => 'foo' ),
array( 'key' => 'bar' ),
);
$array['relation'] = 'AND';

I don't particularly like that code pattern, but it would avoid the sniff flagging the array.

an ignore option would be much better, as I'd only have to set this in the config once.

An ignore option will not be accepted for this sniff as you then get the next request: "this key should only be ignored in combination with that key", "this key should only be ignored in file x.php" etc. This is a road to maintainer pain which I'm not willing to take.

kkmuffme commented 11 months ago

Another way to still use the sniff, but avoid this particular issue is as follows:

I have a nested config array, so this won't work without massively bloating the code.

"this key should only be ignored in combination with that key"

Well, let's cross the bridge when we come to it :-) and start with a simple ignore. Anything further can then be discussed IF anybody really needs that.

"this key should only be ignored in file x.php"

This is already supported by this rule, since this is a native feature of phpcs.


Would you accept a PR for a simple "ignore" case, where all keys in the ignore array are treated as if they're not part of the array at all? (ignored)

jrfnl commented 11 months ago

This is already supported by this rule, since this is a native feature of phpcs.

It is not, as properties are per sniff and not configurable per file.

Would you accept a PR for a simple "ignore" case, where all keys in the ignore array are treated as if they're not part of the array at all? (ignored)

As I've stated three times now: no, I would not.

kkmuffme commented 11 months ago

It is not, as properties are per sniff and not configurable per file.

phpcs:disable in the file comment, does precisely this though?

jrfnl commented 11 months ago

It is not, as properties are per sniff and not configurable per file.

phpcs:disable in the file comment, does precisely this though?

No it does not. It disables the complete sniff for a file. Not apply a specific property exclusively to a specific file.