Add more testing and fix some errors#25
Conversation
|
Whooooaa ! Thank you very very very much for this great PR! I will check it later this day or tomorrow when I have some time. |
| public function __get($name) | ||
| { | ||
| if (in_array($name, array('rho', 'theta', 'r', 'i'))) { | ||
| return $this->$name; |
There was a problem hiding this comment.
Why this change?
This break the possibility to have $object->rho, or $object->theta or $object->r or $object->i direct read access.
There was a problem hiding this comment.
I think directly return $name is appropriate.
If set the original code $this->$name, the variable will return null.
Because this variable is not defined in this class.
There was a problem hiding this comment.
How about declare a private $name and change $this->$name to $this->name?
There was a problem hiding this comment.
I see what you mean, but it is confusing, really.
But this class should be change in futur I think.
There was a problem hiding this comment.
I want keep magic getter to get this values, less confusing I think.
| { | ||
| $f = new Factorial(2); | ||
| $this->assertInternalType('string', $f->__toString()); | ||
| $this->assertSame('2', $f->__toString()); |
There was a problem hiding this comment.
... or just $this->assertSame('2', "$f"); :-)
| public function testGetShouldBePI() | ||
| { | ||
| $real = new Real(2.56); | ||
| $this->assertEquals(M_PI, $real->__get('pi')->__toString()); |
There was a problem hiding this comment.
Hum… I dont know, this is strange to get pi, e, and so on from an instance. Confusing.
But, we can do that:
$real = new Real(Real::PI);
$this->assertEquals(M_PI, "$real");
| public function testGetShouldBeE() | ||
| { | ||
| $real = new Real(2.56); | ||
| $this->assertEquals(M_E, $real->__get('e')->__toString()); |
| public function testGetShouldBeEuler() | ||
| { | ||
| $real = new Real(2.56); | ||
| $this->assertEquals(M_EULER, $real->__get('euler')->__toString()); |
| public function testGetTheCorrectName() | ||
| { | ||
| $r = new RandomComplex(); | ||
| $this->assertEquals('r', $r->__get('r')); |
There was a problem hiding this comment.
No, sorry, but this breaks original behavior. Please see fa42d35#r150422922
| public function testSetWithTheSampleOne() | ||
| { | ||
| $t = new Independant(); | ||
| $t->__set('sample_1', array(1,2,3)); |
There was a problem hiding this comment.
It is a magic setter, so it is better to test it in one shot like this:
$t->sample_1 = array(1,2,3);
| public function testSetWithTheSampleTwo() | ||
| { | ||
| $t = new Independant(); | ||
| $this->assertNull($t->__set('sample_2', array(1,2,3))); |
| public function testSetWithArgumentIsNoStatsInstance() | ||
| { | ||
| $t = new Independant(); | ||
| $t->__set('sample_1', 'no'); |
| public function testGetWithNameIsMidRange() | ||
| { | ||
| $s = new Stats(array(1,2,3,4,5,6,7)); | ||
| $this->assertEquals(4, $s->__get('midrange')); |
There was a problem hiding this comment.
It is a magic getter, so to test it, the best way is to call it:
$this->assertEquals(4, $s->midrange);
| public function testGetWithNameIsPlatykurtic() | ||
| { | ||
| $s = new Stats(array(1,2,3,4,5,6,7)); | ||
| $this->assertEquals(4, $s->__get('is_platykurtic')); |
| public function testGetWithNameIsLeptokurtic() | ||
| { | ||
| $s = new Stats(array(1,2,3,4,5,6,7)); | ||
| $this->assertFalse($s->__get('is_leptokurtic')); |
| public function testGetWithNameIsMesokurtic() | ||
| { | ||
| $s = new Stats(array(1,2,3,4,5,6,7)); | ||
| $this->assertFalse($s->__get('is_mesokurtic')); |
| public function testGetWithNameIsIndexOfDispersion() | ||
| { | ||
| $s = new Stats(array(1,2,3,4,5,6,7)); | ||
| $this->assertEquals(1, $s->__get('coefficient_of_dispersion')); |
| public function testGetWithNameIsPearsonsR() | ||
| { | ||
| $s = new Stats(array(1,2,3,4,5,6,7)); | ||
| $this->assertEquals(1, $s->__get('pearsons_rho')); |
| public function testGetShouldBeType() | ||
| { | ||
| $a = new Angle(4 * pi(), Angle::TYPE_RAD); | ||
| $this->assertEquals('rad', $a->__get('type')); |
There was a problem hiding this comment.
It is a magic getter, so it better to test it as it should be call:
$this->assertEquals('rad', $a->type);
|
Great PR, but I havesome comment like you have see ;-) Many thanks! |
|
Hi @malenkiki, thank you for your review. Thanks! |
2 participants
Thank you for providing this great math package.Here is the changed log is as follows:
Changed log
.travis.yml.set the dist is precise because of this issue. And it can test well in PHP 5.3.
BTW, I think the coding style should be unified.
Perhaps we can use PHP-CS-Fixer or hook the StyleCI service and set the PSR-2 coding style.
What do you think?
Thanks.