-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Config: display user-friendly message when invalid generator is passed #771
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -198,6 +198,23 @@ class Config | |||||||||
*/ | ||||||||||
private static $executablePaths = []; | ||||||||||
|
||||||||||
/** | ||||||||||
* A list of valid generators. | ||||||||||
* | ||||||||||
* - Keys: lowercase version of the generator name. | ||||||||||
* - Values: name of the generator PHP class. | ||||||||||
* | ||||||||||
* Note: once support for PHP < 5.6 is dropped, this property should be refactored into a class | ||||||||||
* constant. | ||||||||||
Comment on lines
+207
to
+208
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
* | ||||||||||
* @var array<string, string> | ||||||||||
*/ | ||||||||||
private static $validGenerators = [ | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for making this property |
||||||||||
'text' => 'Text', | ||||||||||
'html' => 'HTML', | ||||||||||
'markdown' => 'Markdown', | ||||||||||
]; | ||||||||||
|
||||||||||
|
||||||||||
/** | ||||||||||
* Get the value of an inaccessible property. | ||||||||||
|
@@ -1233,7 +1250,21 @@ public function processLongArgument($arg, $pos) | |||||||||
break; | ||||||||||
} | ||||||||||
|
||||||||||
$this->generator = substr($arg, 10); | ||||||||||
$generatorName = substr($arg, 10); | ||||||||||
$lowerCaseGeneratorName = strtolower($generatorName); | ||||||||||
|
||||||||||
if (isset(self::$validGenerators[$lowerCaseGeneratorName]) === false) { | ||||||||||
$validOptions = implode(', ', array_values(self::$validGenerators)); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also wondering if for readability, the last comma should be replaced with " and " ? |
||||||||||
$error = sprintf( | ||||||||||
'ERROR: "%s" is not a valid generator. Valid options are: %s.'.PHP_EOL.PHP_EOL, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a suggestion...
Suggested change
|
||||||||||
$generatorName, | ||||||||||
$validOptions | ||||||||||
); | ||||||||||
$error .= $this->printShortUsage(true); | ||||||||||
throw new DeepExitException($error, 3); | ||||||||||
} | ||||||||||
|
||||||||||
$this->generator = self::$validGenerators[$lowerCaseGeneratorName]; | ||||||||||
self::$overriddenDefaults['generator'] = true; | ||||||||||
} else if (substr($arg, 0, 9) === 'encoding=') { | ||||||||||
if (isset(self::$overriddenDefaults['encoding']) === true) { | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -23,37 +23,59 @@ final class GeneratorArgTest extends TestCase | |||||
/** | ||||||
* Ensure that the generator property is set when the parameter is passed a valid value. | ||||||
* | ||||||
* @param string $generatorName Generator name. | ||||||
* @param string $argumentValue Generator name passed in the command line. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* @param string $expectedPropertyValue Expected value of the generator property. | ||||||
* | ||||||
* @dataProvider dataGeneratorNames | ||||||
* @dataProvider dataValidGeneratorNames | ||||||
* | ||||||
* @return void | ||||||
*/ | ||||||
public function testGenerators($generatorName) | ||||||
public function testValidGenerators($argumentValue, $expectedPropertyValue) | ||||||
{ | ||||||
$config = new ConfigDouble(["--generator=$generatorName"]); | ||||||
$config = new ConfigDouble(["--generator=$argumentValue"]); | ||||||
|
||||||
$this->assertSame($generatorName, $config->generator); | ||||||
$this->assertSame($expectedPropertyValue, $config->generator); | ||||||
|
||||||
}//end testGenerators() | ||||||
}//end testValidGenerators() | ||||||
|
||||||
|
||||||
/** | ||||||
* Data provider for testGenerators(). | ||||||
* Data provider for testValidGenerators(). | ||||||
* | ||||||
* @see self::testGenerators() | ||||||
* @see self::testValidGenerators() | ||||||
* | ||||||
* @return array<int, array<string>> | ||||||
*/ | ||||||
public static function dataGeneratorNames() | ||||||
public static function dataValidGeneratorNames() | ||||||
{ | ||||||
return [ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this array is now a little more involved, I think it would make it more straight-forward to maintain and to read the tests if the array included test case names + argument names. |
||||||
['Text'], | ||||||
['HTML'], | ||||||
['Markdown'], | ||||||
[ | ||||||
'Text', | ||||||
'Text', | ||||||
], | ||||||
[ | ||||||
'HTML', | ||||||
'HTML', | ||||||
], | ||||||
[ | ||||||
'Markdown', | ||||||
'Markdown', | ||||||
], | ||||||
[ | ||||||
'TEXT', | ||||||
'Text', | ||||||
], | ||||||
[ | ||||||
'tEXt', | ||||||
'Text', | ||||||
], | ||||||
[ | ||||||
'html', | ||||||
'HTML', | ||||||
], | ||||||
]; | ||||||
|
||||||
}//end dataGeneratorNames() | ||||||
}//end dataValidGeneratorNames() | ||||||
|
||||||
|
||||||
/** | ||||||
|
@@ -76,4 +98,50 @@ public function testOnlySetOnce() | |||||
}//end testOnlySetOnce() | ||||||
|
||||||
|
||||||
/** | ||||||
* Ensure that an exception is thrown for an invalid generator. | ||||||
* | ||||||
* @param string $generatorName Generator name. | ||||||
* | ||||||
* @dataProvider dataInvalidGeneratorNames | ||||||
* | ||||||
* @return void | ||||||
*/ | ||||||
public function testInvalidGenerator($generatorName) | ||||||
{ | ||||||
$exception = 'PHP_CodeSniffer\Exceptions\DeepExitException'; | ||||||
$message = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, Markdown.'; | ||||||
|
||||||
if (method_exists($this, 'expectException') === true) { | ||||||
// PHPUnit 5+. | ||||||
$this->expectException($exception); | ||||||
$this->expectExceptionMessage($message); | ||||||
} else { | ||||||
// PHPUnit 4. | ||||||
$this->setExpectedException($exception, $message); | ||||||
} | ||||||
|
||||||
new ConfigDouble(["--generator={$generatorName}"]); | ||||||
|
||||||
}//end testInvalidGenerator() | ||||||
|
||||||
|
||||||
/** | ||||||
* Data provider for testInvalidGenerator(). | ||||||
* | ||||||
* @see self::testInvalidGenerator() | ||||||
* | ||||||
* @return array<int, array<string>> | ||||||
*/ | ||||||
public static function dataInvalidGeneratorNames() | ||||||
{ | ||||||
return [ | ||||||
['InvalidGenerator'], | ||||||
['Text,HTML'], | ||||||
[''], | ||||||
]; | ||||||
|
||||||
}//end dataInvalidGeneratorNames() | ||||||
|
||||||
|
||||||
}//end class |
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.
Should this be part of the "long description" of the variable or part of the description of the
@var
type ?