Skip to content
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

feat: authorize adding extra rules, no override allowed #21

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ jobs:
php-version: ${{ matrix.php-version }}
- run: composer install --prefer-dist --no-interaction
- run: vendor/bin/php-cs-fixer fix --ansi --dry-run --using-cache=no --verbose
- run: vendor/bin/phpunit --colors=always
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
vendor/
composer.lock
*.phar
.phpunit.result.cache
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,33 @@ $config->setFinder($finder);
return $config;
```

#### Adding extra rules:

In case you wish to add some more rules (be more restrictive / apply project level rules), the `M6Web\CS\Config\BedrockStreaming` configuration accepts them as a constructor argument.

⚠️ This feature does not allow any override. ⚠️

```php
<?php

$finder = PhpCsFixer\Finder::create()
->in(
[
__DIR__.'/src',
__DIR__.'/tests',
]
);

$config = new M6Web\CS\Config\BedrockStreaming([
'native_function_invocation' => ['scope' => 'all'],
]
);
$config->setFinder($finder);

return $config;
```


### Git

Add `.php-cs-fixer.cache` (this is the cache file created by `php-cs-fixer`) to `.gitignore`:
Expand Down
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
],
"require": {
"php": "^7.1.3 || ^8.0",
"friendsofphp/php-cs-fixer": "^3.0"
"friendsofphp/php-cs-fixer": "^3.1",
"phpunit/phpunit": "^9.6"
},
"autoload": {
"psr-4": {
Expand Down
22 changes: 22 additions & 0 deletions phpunit.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/8.0/phpunit.xsd"
bootstrap="vendor/autoload.php"
executionOrder="depends,defects"
forceCoversAnnotation="true"
beStrictAboutCoversAnnotation="true"
beStrictAboutOutputDuringTests="true"
beStrictAboutTodoAnnotatedTests="true"
verbose="true">
<testsuites>
<testsuite name="default">
<directory suffix="Test.php">tests</directory>
</testsuite>
</testsuites>

<filter>
<whitelist processUncoveredFilesFromWhitelist="true">
<directory suffix=".php">src</directory>
</whitelist>
</filter>
</phpunit>
12 changes: 9 additions & 3 deletions src/BedrockStreaming.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,20 @@

final class BedrockStreaming extends Config
{
public function __construct()
/** @var array */
private $extraRules;

public function __construct(array $extraRules = [])
{
parent::__construct('Bedrock Streaming');

$this->extraRules = $extraRules;
$this->setRiskyAllowed(true);
}

public function getRules(): array
{
$rules = [
$standardRules = [
'@Symfony' => true,
'array_syntax' => [
'syntax' => 'short',
Expand Down Expand Up @@ -46,6 +50,8 @@ public function getRules(): array
'yoda_style' => false,
];

return $rules;
$newRules = \array_diff_key($this->extraRules, $standardRules);

return \array_merge($newRules, $standardRules);
Comment on lines +53 to +55
Copy link
Member

@Oliboy50 Oliboy50 May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ adding a new item to the $standardRules will be a potential breaking change for those who uses $extraRules

example:

  1. Toto uses new BedrockStreaming(['explicit_string_variable' => true])
  2. 'explicit_string_variable' => false is added to the $standardRules
  3. Toto updates its dependencies and then he cries because its code does not work anymore... Will it pin its dependencies to the previous version? 👀

now, the question for this repository maintainers will be to define if they should bump a new major release each time they add an item to $standardRules or minor release like before

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can take the problem by another angle.
Why not, have an empty configuration and create a BR CS who decorate this empty config. And if a project or team want the same BR CS rules and add his own rule, they could just decorate the BR CS config. And why not add a overrideRules parameter that allow to override all decorated config.

Some pseudocode:
empty config

class EmptyConfig {
    contructor () {
        this->rules = []
    }
}
config = new Config()

BR CS config

class BRCS {
    constructor(config Config, array extraRule, bool overrideRules) {
        this->rule = ["rule1" => true]
        merge rules with override or not
    }
}
config = new BRCS(new Config)

project1 of BR config

class Project1BR {
    constructor(config Config, array extraRule, bool overrideRules) {
        this->rule = ["rule2" => false]
        merge rules with override or not
    }
}
config = new Project1BR(new BRConfig(new Config), ["rule1" => false], true)

It's just pseudocode, and I miss certainly a lot of things, but the idea is here.
What to you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion!

I think I understand your suggestion. Technically, it looks ok (although, I think we must forbid overriding). Using decorators looks clean.

However, I think it's a bit overkill regarding the original proposition where we (my team) wish to add a few extra rules to the company rules.

Maybe you've got a more complex use case in mind?

}
}
45 changes: 45 additions & 0 deletions tests/BedrockStreamingTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

declare(strict_types=1);

use M6Web\CS\Config\BedrockStreaming;
use PHPUnit\Framework\TestCase;

class BedrockStreamingTest extends TestCase
{
/**
* @test
* @covers M6Web\CS\Config\BedrockStreaming::getRules
*/
public function RulesOrderShoulNotBeAltered()
{
$extraRules = [
'braces' => [
'allow_single_line_closure' => false,
],
'dummy_rule' => 'dummy_value',
'@Symfony' => false,
];
$rules = (new BedrockStreaming($extraRules))->getRules();

self::assertEquals('array', gettype($rules));

// Make sure the new rule is added before the standard ones to prevent any regression
$firstValue = reset($rules);
$firstKey = key($rules);
self::assertEquals('dummy_rule', $firstKey, 'The new rules should be added before standard ones');
self::assertEquals('dummy_value', $firstValue);

// Make sure the first standard rule comes after the custom rules
self::assertArrayHasKey('@Symfony', $rules, 'The @Symfony rule should not be deleted');
$nextValue = next($rules);
$nextKey = key($rules);
self::assertEquals('@Symfony', $nextKey, 'The first rule should always remain @Symfony');
self::assertTrue($nextValue, 'The standard root rules value should never change');

// Make sure a nested rule cannot be overriden
self::assertArrayHasKey('braces', $rules);
self::assertArrayHasKey('allow_single_line_closure', $rules['braces']);
self::assertTrue($rules['braces']['allow_single_line_closure'], 'The standard nested rules should never change');
}
}