findologic / findologic-api

Library for sending requests to the Findologic API
MIT License
1 stars 3 forks source link

Review purposes only #12

Closed TheKeymaster closed 5 years ago

TheKeymaster commented 6 years ago

@georgms I have seen that you have not accepted the invite to this repository, but here is the link https://github.com/TheKeymaster/findologic-api/invitations.

Also I have created this branch after the GitHub support wrote me how it is possible that all changes are in one branch. Since you have not accepted the invite yet, I can't add you as reviewer for this repository, but I would be glad if you could review my code.

Thanks!

codecov[bot] commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (review-branch@b51f6a6). Click here to learn what that means. The diff coverage is 99.77%.

Impacted file tree graph

@@               Coverage Diff                @@
##             review-branch      #12   +/-   ##
================================================
  Coverage                 ?   99.77%           
  Complexity               ?      190           
================================================
  Files                    ?       23           
  Lines                    ?      442           
  Branches                 ?        0           
================================================
  Hits                     ?      441           
  Misses                   ?        1           
  Partials                 ?        0
Impacted Files Coverage Δ Complexity Δ
...rc/FINDOLOGIC/Objects/XmlResponseObjects/Limit.php 100% <100%> (ø) 3 <3> (?)
src/FINDOLOGIC/Objects/XmlResponseObjects/Item.php 100% <100%> (ø) 12 <12> (?)
src/FINDOLOGIC/Objects/XmlResponse.php 100% <100%> (ø) 16 <16> (?)
src/FINDOLOGIC/Definitions/RequestType.php 100% <100%> (ø) 1 <1> (?)
...rc/FINDOLOGIC/Objects/XmlResponseObjects/Query.php 100% <100%> (ø) 8 <8> (?)
src/FINDOLOGIC/Exceptions/ParamNotSetException.php 100% <100%> (ø) 1 <1> (?)
src/FINDOLOGIC/Objects/JsonResponse.php 100% <100%> (ø) 3 <3> (?)
...NDOLOGIC/Objects/XmlResponseObjects/Attributes.php 100% <100%> (ø) 5 <5> (?)
.../FINDOLOGIC/Objects/XmlResponseObjects/Results.php 100% <100%> (ø) 2 <2> (?)
src/FINDOLOGIC/FindologicApi.php 100% <100%> (ø) 35 <35> (?)
... and 13 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 b51f6a6...f60de0c. Read the comment docs.

TheKeymaster commented 6 years ago

@georgms Wow thank you very much for this detailed and also quick response. I really did not expect that you would go so far, that even GitHub told you that this review is just ridiculously huge!

image (This image was made possible with a custom dark theme.. I like dark themes x))

Due to school I cannot immediately get to your review but I really want to say thank you for investing that much time for it. I will definitely take my time to look over your suggestions.

georgms commented 6 years ago

@georgms Wow thank you very much for this detailed and also quick response. I really did not expect that you would go so far, that even GitHub told you that this review is just ridiculously huge!

image (This image was made possible with a custom dark theme.. I like dark themes x))

Due to school I cannot immediately get to your review but I really want to say thank you for investing that much time for it. I will definitely take my time to look over your suggestions.

Glad to be of service! Let me know when it's time for another review round.

TheKeymaster commented 5 years ago
TheKeymaster commented 5 years ago

Review issues solved

All issues that were to big for a review, were moved to an issue and will be fixed later. image

@georgms it would be great if you could review my changes! You can find them here in this PR https://github.com/TheKeymaster/findologic-api/pull/13.
Thank you!!

georgms commented 5 years ago

@TheKeymaster Can this PR be closed? Too many PRs always confuses me.

TheKeymaster commented 5 years ago

@georgms yes I will close this PR.