Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Conversation

AndrolGenhald
Copy link
Contributor

Fix issue #87

DateStepTest->testMoscowWinterTime started failing when I was working on this, but it fails on master now too.

@Maks3w
Copy link
Member

Maks3w commented Jun 16, 2016

The proposed changes breaks current tests. Could you review it?

@@ -27,6 +27,33 @@ public function setUp()
$this->validator = new NotEmpty();
}

public function testConstructorWithTypeArray()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refactor this as a data provider.

@Maks3w
Copy link
Member

Maks3w commented Jun 17, 2016

The coverage drop is normal, less lines per file leads to a higher percent per (uncovered) line.

$options['type'] = $detected;
}
if (($type = $this->calculateTypeValue($options)) != 0) {
$options['type'] = $type;
Copy link
Member

@Maks3w Maks3w Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better create a new $options array here for to discard all those old values are now represented by $type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break options for AbstractValidator. I checked and it doesn't actually cause testTypeAutoDetectionHasNoSideEffect to fail, so maybe another test should be added that uses an array that has both types and AbstractValidator options? Either that or document that if the array has any non-type values it will just be treated as a normal options array.
It looks like the current constructor allows type strings to be combined with other options.

@Maks3w
Copy link
Member

Maks3w commented Jun 17, 2016

I appreciate the constructor needs more improvements. calculateTypeValue is called at least 3 times.

  1. $this->setType($this->defaultType);
  2. $this->calculateTypeValue($options));
  3. parent::__construct($options); // if $options has the option key type

Could you take a look to this?

@AndrolGenhald
Copy link
Contributor Author

calculateTypeValue should be called at most twice now, where the second call will only check 2 conditionals before returning. I don't think it's possible to guarantee that it'll only be called once, as there has to be some way to tell if the options array directly contains types.

Xerkus added a commit that referenced this pull request Mar 17, 2017
@Xerkus Xerkus closed this in 85fb5aa Mar 17, 2017
Xerkus added a commit that referenced this pull request Mar 17, 2017
@Xerkus Xerkus added this to the 2.9.0 milestone Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants