-
Notifications
You must be signed in to change notification settings - Fork 0
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
Approve code review #6
Changes from 5 commits
adbdeab
fc13c82
2cc0eff
a3b1487
9c41678
358db04
b445fe0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,13 +14,15 @@ class GetSpecificRate implements ProviderResource | |
{ | ||
private $apiKey; | ||
|
||
public $base = 'BTC'; | ||
private $base; | ||
|
||
public $quote = 'USD'; | ||
private $quote; | ||
|
||
public function __construct(string $apiKey) | ||
public function __construct(string $apiKey, string $base = 'BTC', string $quote = 'USD') | ||
{ | ||
$this->apiKey = $apiKey; | ||
$this->base = $base; | ||
$this->quote = $quote; | ||
} | ||
|
||
public function getProviderClassName(): string | ||
|
@@ -30,7 +32,7 @@ public function getProviderClassName(): string | |
|
||
public function fetch(ImportConnector $connector): \Iterator | ||
{ | ||
$response = \json_decode( | ||
$response[] = \json_decode( | ||
(string) $connector->fetch( | ||
CryptoMonitor::buildExchangeApiUrl( | ||
sprintf("v1/exchangerate/%s/%s", $this->base, $this->quote) | ||
|
@@ -39,15 +41,6 @@ public function fetch(ImportConnector $connector): \Iterator | |
true | ||
); | ||
|
||
$rate = function () use ($response) { | ||
yield [ | ||
'asset_id_base' => $response['asset_id_base'], | ||
'asset_id_quote' => $response['asset_id_quote'], | ||
'rate' => $response['rate'], | ||
'time' => $response['time'], | ||
]; | ||
}; | ||
|
||
return new SpecificRateRecord($rate(), count($response), $this); | ||
return yield $response; | ||
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.
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,31 +17,30 @@ final class GetRateTest extends TestCase | |
|
||
public function testGetSpecificRateRecords() | ||
{ | ||
/** @var SpecificRateRecord $rates */ | ||
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetSpecificRate($this->apiKey))) | ||
->findFirstCollection(); | ||
$rate = FixtureFactory::createPorter()->importOne(new ImportSpecification(new GetSpecificRate($this->apiKey))); | ||
|
||
$rateRecords = $rates->toAssociativeArray(); | ||
$this->assertCount(1, $rate); | ||
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. This is unnecessary because |
||
|
||
$this->assertCount(1, $rateRecords); | ||
$this->assertArrayHasKey('asset_id_base', $rateRecords[0]); | ||
$this->assertArrayHasKey('asset_id_quote', $rateRecords[0]); | ||
$this->assertArrayHasKey('rate', $rateRecords[0]); | ||
$this->assertArrayHasKey('time', $rateRecords[0]); | ||
$this->assertArrayHasKey('asset_id_base', $rate[0]); | ||
$this->assertArrayHasKey('asset_id_quote', $rate[0]); | ||
$this->assertArrayHasKey('rate', $rate[0]); | ||
$this->assertArrayHasKey('time', $rate[0]); | ||
} | ||
|
||
public function testGetAllRateRecords() | ||
{ | ||
/** @var AllRatesRecord $rates */ | ||
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetAllRate($this->apiKey))) | ||
->findFirstCollection(); | ||
$rates = FixtureFactory::createPorter()->import(new ImportSpecification(new GetAllRate($this->apiKey))); | ||
|
||
$this->assertSame('BTC', $rates->getBase()); | ||
$this->assertCount(1267, $rates); | ||
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. This magic number is too brittle. The test needs to prove the count is correct without breaking whenever a rate is added or removed. You have two options, either:
|
||
|
||
$rateRecords = $rates->toAssociativeArray(); | ||
foreach ($rates as $rateRecord) { | ||
$this->assertArrayHasKey('asset_id_quote', $rateRecord); | ||
$this->assertArrayHasKey('rate', $rateRecord); | ||
$this->assertArrayHasKey('time', $rateRecord); | ||
} | ||
|
||
$this->assertArrayHasKey('asset_id_quote', $rateRecords[0]); | ||
$this->assertArrayHasKey('rate', $rateRecords[0]); | ||
$this->assertArrayHasKey('time', $rateRecords[0]); | ||
$rates = $rates->findFirstCollection(); | ||
$this->assertSame('BTC', $rates->getBase()); | ||
} | ||
} |
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.
This is wrapping your record in an outer array, which is not correct. This should just be
$response
instead of$response[]
.