Add timeframe to hourly #4
Conversation
jaap
force-pushed
the
add-timeframe-to-hourly
branch
from
d64a2a8
to
13aad9f
Compare
|
This PR also contains the changes from #1 which makes it hard to review. Can these be reverted from this PR? |
It didn't run on my system, so changed importing
Resolves ricbra#3
jaap
force-pushed
the
add-timeframe-to-hourly
branch
from
13aad9f
to
97516b3
Compare
|
I updated my branch and removed 88459b8. |
|
Ok, now checks fail of course. Sorry for making such a mess here. |
|
Cool, could take a look at my comments and fix them in the new PR? Thanks for your patience again :) |
|
For some reason I can't find the comments you're mentioning. Am I overlooking something? |
|
Strange, I did check there (and did it again just now), no comments visible to me 😥 |
| { | ||
|
|
||
| $start = $start->format('Ymd') . sprintf("%02d", $timeframe_start); |
There was a problem hiding this comment.
Maybe we should just completely drop the DateTime usage here as KNMI expects an alternative format anyway. So:
public function getHourly(string $timeframeStart, string $timeframeEnd, array $stations, array $vars) : arrayI don't like adding another two params here as it can be confusing. Less params is better.
There was a problem hiding this comment.
Thinking about it, but for 'day selection' using regular dates is IMO easier then formatting it into a string on forehand. The separate timeframe option allows you to set the timeframes you'd wish to collect from those days. Doing it your way you'd still be formatting the start, end and timeframe into 2 strings.
Doing it by using my proposal you can still use regular date objects and just collect only morning data by providing $timeframe_start = 6, and $timeframe_end = 12.
Do you get what I mean? Or am I understanding you wrong?
There was a problem hiding this comment.
Yeah, you got a point. It's just confusing design from the KNMI 😄.
Could you change the vars to camelCase:
$timeframeStart
|
|
||
| class ClientTest extends \PHPUnit_Framework_TestCase | ||
| class ClientTest extends TestCase |
There was a problem hiding this comment.
Could you revert this change as it is not required? We're depending on phpunit ^5.6 which still has the old non-namespaced class names. If you run the tests with the composer installed phpunit, it should work with the non-namespaced classes (vendor/bin/phpunit).
| @@ -137,4 +139,26 @@ public function it_correctly_calls_and_parses_knmi_hourly_response() | |||
| $response | |||
| ); | |||
| } | |||
| /** | |||
| $parsed[0]['station']['name'] | ||
| ); | ||
| } | ||
|
|
| * @group integration | ||
| * @test | ||
| */ | ||
| public function it_correctly_calls_and_parses_station_names_with_specia_characters() |
|
|
||
| class ResponseParserTest extends \PHPUnit_Framework_TestCase | ||
| class ResponseParserTest extends TestCase |
There was a problem hiding this comment.
Please revert, see my earlier comment in the other test case
|
I just submitted another review with all the comments, you should be able to see them now. My previous reviews weren't submitted, my bad. |
Resolves #3