-
Notifications
You must be signed in to change notification settings - Fork 18
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
Added config validation #34
Added config validation #34
Conversation
composer.json
Outdated
"phpstan/phpstan": "1.11.x-dev" | ||
"phpstan/phpstan": "1.11.x-dev", | ||
"phpstan/phpstan-symfony": "1.4.x-dev", | ||
"friendsofphp/php-cs-fixer": "^3.38" |
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.
What is the purpose of this? I don't think it's needed, because we don't use it as a dependency and install it via third-party on CI. Locally, it's probably a good idea to have it installed globally, not per project.
In other words, we don't "depend" on those libs directly, they are just used on CI to help, but nothing more. It seems best practice to not require it in composer.json anymore. Actually, I think the same relates to PHPStan as well, we can completely remove those 2 libs and if CI is happy - double win, fewer dependencies.
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.
The idea was to prevent checks to fail before a PR was created and since phpstan
was already a dev-dependency, but changed it.
I've now added only phpstan/phpstan-symfony
as requested.
@@ -1,10 +1,7 @@ | |||
includes: | |||
- vendor/phpstan/phpstan-symfony/extension.neon |
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.
Though probably if we want to link this directly from the vendor - it may require the phpstan/phpstan-symfony
pack, but probably only that one, phpstan/phpstan
is redundant I think.
Btw, I suppose this line fixed that ignored error message you deleted below, right?
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.
Yes it did :)
return true; | ||
} | ||
|
||
$filename = $currentFilename; |
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.
Hm, I'm not sure this solution is robust enough... it works for 2 files, but with more files it will not work because it only reveals duplicates that are going consequently.
I think we need to collect all the filenames in an array, and every time check if that array already contains the currently handled filename. It should be more robust, but still it might be a possible problem later if the upstream logic changed :/
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 (again) being sharper than me, fixed it and even added tests for the configuration
b601d12
to
d3f0684
Compare
6cdbf05
to
d82f703
Compare
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 adding some tests! I like this, just a minor error message tweak
04ebb75
to
53cc09d
Compare
Thank you |
Currently with the following config:
Running the
sass:build
command would result all configured paths to be build in 1 file;var/sass/app.output.css
, as allroot_sass
paths with the same filename will be stored under the same css filename, thus overwriting the file.This PR throws an exception when configuring the sass-paths. (follow-up on #30)