-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
POC Add support for passing multiple files to fix when rules has been giv… #4408
POC Add support for passing multiple files to fix when rules has been giv… #4408
Conversation
|
||
$output = $cmdTester->getDisplay(true); | ||
|
||
static::assertNotFalse(strpos($output, 'Loaded config default.')); |
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.
it's not obvious why loading 2 unrelated things (fixing multiple files + passing rules by cli param) is making tool to ignore the config file.
php-cs-fixer fix src/path1 src/path2 --rules=foo
and boom, whole .php_cs.dist
got ignored
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.
Are you claiming php-cs-fixer fix src/path1 src/path2 --rules=foo
runs currently? Cause it does not.
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.
i'm not claiming. i'm saying.
after change proposed with this PR, running given command would ignore configuration file totally, which is wrong.
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.
running given command would ignore configuration file totally, which is wrong.
"wrong" given your specs, I think it is totally fine
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.
if we ignore the config, we shall at least give user the note about it.
i don't want any implicit actions simply.
remember that we configure not only rules in config file.
maybe let's have the --config=default
or --no-config
to allow for such cases?
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.
Passing both
config
andrules
options is not possible.
It is already implicit which config file will be used when you use rules
because you are not allowed to pass one explicit. So making it explicit the default config with the commandline arguments override seems best to me.
(on 3.x for BC sake)
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.
yeah, i guess it was a preparation for killing --rules in general
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.
i don't see how this gonna work, at least in current state
reasons are the same as in #4411
ok |
…en as well.
@ see #4279 (comment)