Codecov Report

Merging #23 into dev will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##              dev    #23   +/-   ##
=====================================
  Coverage     100%   100%           
- Complexity    191    217   +26     
=====================================
  Files          27     32    +5     
  Lines         514    594   +80     
=====================================
+ Hits          514    594   +80

Impacted Files	Coverage Δ	Complexity Δ
...OGIC/Api/ResponseObjects/Xml20/Properties/Item.php	`100% <ø> (ø)`	`12 <0> (?)`
...onseObjects/Autocomplete/Properties/Suggestion.php	`100% <ø> (ø)`	`11 <0> (?)`
...Api/ResponseObjects/Xml20/Properties/Promotion.php	`100% <ø> (ø)`	`3 <0> (?)`
...GIC/Api/ResponseObjects/Xml20/Properties/Limit.php	`100% <ø> (ø)`	`3 <0> (?)`
...pi/ResponseObjects/Xml20/Properties/Attributes.php	`100% <ø> (ø)`	`5 <0> (?)`
...i/ResponseObjects/Xml20/Properties/LandingPage.php	`100% <ø> (ø)`	`2 <0> (?)`
...C/Api/ResponseObjects/Xml20/Properties/Servers.php	`100% <ø> (ø)`	`3 <0> (?)`
...GIC/Api/ResponseObjects/Xml20/Properties/Query.php	`100% <ø> (ø)`	`8 <0> (?)`
...ResponseObjects/Xml20/Properties/OriginalQuery.php	`100% <ø> (ø)`	`3 <0> (?)`
...C/Api/ResponseObjects/Xml20/Properties/Product.php	`100% <ø> (ø)`	`7 <0> (?)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8413eff...8dfa929. Read the comment docs.

TheKeymaster commented 5 years ago

@howard

If you want and if you have time in near future, please take a look at the future of FINDOLOGIC-API and review this beast of a Pull Request :).

TheKeymaster commented 5 years ago

@howard

I fundamentally disagree with the design decision that request builders make strings that are then used by the client. Why not build request objects that are passed to the client in a type safe way? This would prevent people from manually tampering with the generated URL and reduces risk of incorrect use.

Please give me some kind of example on how your thought process is. Do you mean I should pass $this when trying to call the request method of the Client as a RequestBuilder? This was the real reason behind it because I think it is damn dirty to pass $this in any kind of situation. Maybe your thought process was a bit a different one, but that was just what I understood. Please give me an example or something that I could work with. Maybe we can also just discuss this in short.

Thanks!

howard commented 5 years ago

@TheKeymaster Maybe this bit of pseudo code illustrates my idea. If not, let's discuss this in person.

$config = new Config();
$config->setServiceId('asd');

$client = new Client($config);

$request = new SuggestRequest();
$request->setQuery('foo');

/** @var SuggestResponse $response */
$response = $client->send($request);

$aliveRequest = new AliveTestRequest();
/** @var AliveTestResponse $response */
$aliveResponse = $client->send($aliveRequest);

TheKeymaster commented 5 years ago

@howard feel free to check out the changes that I've made since the last review. It would be awesome if you could find time. I really want to push this forward :).

TheKeymaster commented 5 years ago

@howard @georgms please check the code until Friday, 25th May. If everything is okay from your side I want to merge this finally.

georgms commented 5 years ago

@howard @georgms please check the code until Friday, 25th May. If everything is okay from your side I want to merge this finally.

I will gladly do it this weekend, but I will unfortunately not have time for a proper review before that.

findologic / findologic-api

Rewrite/Restructure the FindologicApi layer #23

Codecov Report