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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default preset "use in annotation" and fix merge config #196

Merged

Conversation

@Jibbarth
Copy link
Collaborator

commented Jun 23, 2019

Q A
Bug fix? yes
New feature? no
Fixed tickets #193 (and maybe #143 #145)

馃 This one may be controversial

I wanted to fix #193 by adding the config for SlevomatCodingStandard\Sniffs\Namespaces\UnusedUsesSniff (with searchAnnotation to true) in DefaultPreset

But I noticed that when we use an other framework preset, the config doesn't extend the default config.

After viewing issues #145, #143 and PR #111 and #147 I tried to fix them.

I before create a test that should cover all case (ie add an exclude path, and override a config value)

With the array_replace_recursive, we can override a config value, but we override also excluded path, so User had to rewrite the all list of exclude to bypass this problem.

But with array_merge_recursive, the values passed in config for a sniff was joined into an array, that was not wanted also.

As @dol mentionned here and here the best option was to write a custom merge function that take care of numeric key.

Once it was done, I also used this function to merge frameworks preset with the Default one.

@nunomaduro If you prefer I just focus on #193, I can remove the last three commits and only add UnusedUseSniff config in DefaultPreset.

@Jibbarth Jibbarth added the bug label Jun 23, 2019

@Jibbarth Jibbarth requested review from nunomaduro and olivernybroe Jun 23, 2019

@olivernybroe
Copy link
Collaborator

left a comment

I haven't tried out myself if the mergeConfig method works as we want, but I expect it to do :)
(if the test passes then it does what I want)

Else I just have some minor changes to the styling.

src/Application/ConfigResolver.php Outdated Show resolved Hide resolved
src/Application/ConfigResolver.php Show resolved Hide resolved
tests/Application/ConfigResolverTest.php Outdated Show resolved Hide resolved
@olivernybroe
Copy link
Collaborator

left a comment

I think this looks pretty good.
However before merge I think we need a approve from @nunomaduro also.

@nunomaduro
Copy link
Owner

left a comment

Hey buddy, I don't get this. When I have developed this code, the default preset was always used. It's not the case today?

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

Hey buddy, I don't get this. When I have developed this code, the default preset was always used. It's not the case today?

I don't think so. I haven't been able to find anywhere were the default preset would get merged with the selected preset.
I might just be blind tbh.馃槄

@Jibbarth

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 24, 2019

@nunomaduro When we change default preset for an other, or if we let the ConfigResolver guess to use another preset, we don't have the default preset AND the framework preset.

For example, if I dump the config in AnalyseCommand, I get:

array:5 [
  "exclude" => array:4 [
    0 => "bower_components"
    1 => "node_modules"
    2 => "vendor"
    3 => ".phpstorm.meta.php"
  ]
  "add" => []
  "remove" => []
  "config" => array:3 [
    "SlevomatCodingStandard\Sniffs\Commenting\DocCommentSpacingSniff" => array:1 [
      "linesCountBetweenDifferentAnnotationsTypes" => 1
    ]
    "SlevomatCodingStandard\Sniffs\TypeHints\DeclareStrictTypesSniff" => array:2 [
      "newlinesCountBetweenOpenTagAndDeclare" => 2
      "spacesCountAroundEqualsSign" => 0
    ]
    "PHP_CodeSniffer\Standards\Generic\Sniffs\Files\LineLengthSniff" => array:2 [
      "lineLimit" => 80
      "absoluteLineLimit" => 120
    ]
  ]
  "preset" => "default"
]

But if I change in phpinsights.php the preset to use the Laravel one, I get:

array:5 [
  "exclude" => array:11 [
    0 => "config"
    1 => "storage"
    2 => "resources"
    3 => "bootstrap"
    4 => "nova"
    5 => "database"
    6 => "server.php"
    7 => "_ide_helper.php"
    8 => "_ide_helper_models.php"
    9 => "app/Providers/TelescopeServiceProvider.php"
    10 => "public"
  ]
  "add" => []
  "remove" => []
  "config" => array:4 [
    "NunoMaduro\PhpInsights\Domain\Insights\ForbiddenDefineGlobalConstants" => array:1 [
      "ignore" => array:1 [
        0 => "LARAVEL_START"
      ]
    ]
    "PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\ForbiddenFunctionsSniff" => array:1 [
      "forbiddenFunctions" => array:2 [
        "dd" => null
        "dump" => null
      ]
    ]
    "NunoMaduro\PhpInsights\Domain\Sniffs\ForbiddenSetterSniff" => array:1 [
      "allowedMethodRegex" => array:1 [
        0 => "/^set.*?Attribute$/"
      ]
    ]
    "PHP_CodeSniffer\Standards\Generic\Sniffs\Files\LineLengthSniff" => array:2 [
      "lineLimit" => 80
      "absoluteLineLimit" => 120
    ]
  ]
  "preset" => "laravel"
]

As you can see, all exclude path defined in Default are present in laravel preset. Same for the config of DocCommentSpacingSniff and DeclareStrictTypesSniff

I tried to change version to v1.0.0 to see if we had a regression, but I get the same comportment.

@nunomaduro nunomaduro merged commit 1b9217e into nunomaduro:master Jun 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jun 24, 2019

Sounds good. Thank you so much fir this!

@Jibbarth Jibbarth deleted the Jibbarth:fix/default-preset-use-in-annotation branch Jun 24, 2019

@Jibbarth Jibbarth referenced this pull request Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.