-
Notifications
You must be signed in to change notification settings - Fork 19
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
Split all the validators #84
Conversation
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.
So clean 🧐
public function validate(string $password): bool { | ||
$enforceNonCommonPassword = $this->config->getEnforceNonCommonPassword(); | ||
if($enforceNonCommonPassword) { | ||
$passwordFile = __DIR__ . '/../../lists/list-'.strlen($password).'.php'; |
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.
move the $passwordFile
assigment one line up and combine the first two if's for better readibility
['banana', false, true], | ||
['bananabananabananabanana', false, true], | ||
['banana', true, false], | ||
['bananabananabananabanana', true, true], |
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.
😆
@@ -66,7 +66,8 @@ public function testValidate(string $password, bool $enforced, bool $valid) { | |||
$this->expectException(HintException::class); | |||
} | |||
|
|||
$this->assertTrue($this->validator->validate($password)); | |||
$this->assertTrue(true); |
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.
Really? What if if fails?
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.
Well it either throws an exception or not.
The assert is there to ensure phpunit is quite about risky tests
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 was referring to a joke http://geek-and-poke.com/geekandpoke/2014/1/15/philosophising-geeks
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
4000ea6
to
776583e
Compare
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
776583e
to
7d92b2c
Compare
Cleans it up a bit.