-
Notifications
You must be signed in to change notification settings - Fork 5
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,4 @@ | ||
vendor/ | ||
composer.lock | ||
*.phar | ||
.phpunit.result.cache |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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'); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
ℹ️ adding a new item to the
$standardRules
will be a potential breaking change for those who uses$extraRules
example:
new BedrockStreaming(['explicit_string_variable' => true])
'explicit_string_variable' => false
is added to the$standardRules
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 beforeThere 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.
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
BR CS config
project1 of BR config
It's just pseudocode, and I miss certainly a lot of things, but the idea is here.
What to you think ?
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.
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?