Merge Chullo and FedoraApi classes #70
Conversation
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
============================================
+ Coverage 93.2% 96.77% +3.57%
+ Complexity 23 12 -11
============================================
Files 2 1 -1
Lines 103 62 -41
============================================
- Hits 96 60 -36
+ Misses 7 2 -5
Continue to review full report at Codecov.
|
test/CreateResourceTest.php
Outdated
| $result = $client->createResource(""); | ||
| $this->assertNull($result); | ||
| $result = $api->createResource(""); | ||
| $this->assertEquals(404, $result->getStatusCode()); |
There was a problem hiding this comment.
@whikloj raised a good question on his PR, are these useful any more? Maybe codecov will tell us?
There was a problem hiding this comment.
As FedoraApi just returns the Response then a successful mocked request is as useful as a non-successful one. I think most of the ...returnsNullOtherwise and ...returnsFalseOtherwise tests could be dumped with no effect on code coverage.
There was a problem hiding this comment.
@whikloj I'll do a test on one, and see what happens.
| * @return \EasyRdf_Graph | ||
| */ | ||
| public function getGraph(ResponseInterface $response) | ||
| { |
There was a problem hiding this comment.
@dannylamb you want this to be parseGraph?
There was a problem hiding this comment.
maybe? I'll leave it to your judgement. If you think getGraph makes sense then its fine.
|
@whikloj looks like you're right. I'll reap some more code. |
| * @return \EasyRdf_Graph | ||
| */ | ||
| public function getGraph(ResponseInterface $response) | ||
| { |
There was a problem hiding this comment.
maybe? I'll leave it to your judgement. If you think getGraph makes sense then its fine.
src/FedoraApi.php
Outdated
| $options['body'] = $turtle; | ||
|
|
||
| // Save it. | ||
| return $this->client->request( |
There was a problem hiding this comment.
you can just call saveResource() here instead of setting the guzzle options manually
src/FedoraApi.php
Outdated
| $options['body'] = $turtle; | ||
|
|
||
| // Save it. | ||
| return $this->saveResource($uri, $turtle); |
There was a problem hiding this comment.
@dannylamb I think this is right. It passes the test, but I'm not confident.
There was a problem hiding this comment.
You don't need the $options array at all, and you need to pass the headers in to saveResource()
| $guzzle = new Client(['handler' => $handler]); | ||
| $api = new FedoraApi($guzzle); | ||
|
|
||
| $result = $api->createGraph(new \EasyRdf_Graph()); |
There was a problem hiding this comment.
So this test raises a question, should we create a resource if the Graph is empty?
There was a problem hiding this comment.
Oh boy. We probably shouldn't, and I can't think of reason why would want to do that. @dannylamb @jonathangreen thoughts?
There was a problem hiding this comment.
Good question. I'm thinking it's ok to create without content because it would be like an empty POST. But if the graph is empty we should probably just be passing NULL into createResource(). I don't actually know what a serialized empty graph in turtle will return. A blank string? Some prefix declarations?
There was a problem hiding this comment.
This can also totally just be a follow up issue.
| $this->assertFalse($result); | ||
| } | ||
| $result = $api->saveGraph(new \EasyRdf_Graph()); | ||
| $this->assertEquals(204, $result->getStatusCode()); |
There was a problem hiding this comment.
Same question as above, do we generate a resource on an empty graph or do we require some content before we persist it to the repository?
|
I'm good with this. @whikloj @jonathangreen I'm comfortable merging if y'all are ok with it. I'll make a follow up issue to deal with empty graph behaviour. |
GitHub Issue: Resolves Islandora/documentation#601
What does this Pull Request do?
Merges Chullo and FedoraApi classes. Removes Chullo and IFedoraClient.
What's new?
getGraphutility functionHow should this be tested?
Tests should take care of it. But, a good set of eyes from @dannylamb, @jonathangreen and @whikloj should really do it.