Skip to content
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

Add factory methods for DocumentClient, ResponseParser and DocumentParser #79

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

JaZo
Copy link
Member

@JaZo JaZo commented Aug 13, 2020

Description

I've added factory methods to DocumentClient, ResponseParser and DocumentParser. All other parsers are internal and should not be used by consumers, so I didn't add a factory method to those.

Motivation and context

As seen in #74 (comment) setting up the DocumentClient or ResponseParser without a container is quit cumbersome and the user doesn't have to be bothered with internal implementation details. With these factory methods that example can be reduced to this:

<?php
require '../vendor/autoload.php';

$client = \Swis\JsonApi\Client\DocumentClient::create();

$endpointUrl = 'https://cms.contentacms.io/api/recipes';
$response = $client->get($endpointUrl);

/** @var Swis\JsonApi\Client\Collection $collection */
$collection = $response->getData();

// While there is a "next" link there are more pages
while ($response->getLinks()->offsetExists('next')) {
  $response = $client->get($response->getLinks()->offsetGet('next')->getHref());

  // Merge the next page with the current collection
  $collection = $collection->merge($response->getData());
}

/** @var \Swis\JsonApi\Client\Item $item */
foreach ($collection as $item) {
  // Do stuff with the items
  if ($item->hasAttribute('title')) {
    echo $item->title . '<br>';
  }
}

How has this been tested?

Added unit tests and example above.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have read the CONTRIBUTING document.
  • My pull request addresses exactly one patch/feature.
  • I have created a branch for this patch/feature.
  • Each individual commit in the pull request is meaningful.
  • I have added tests to cover my changes.
  • If my change requires a change to the documentation, I have updated it accordingly.

@JaZo JaZo requested a review from bbrala August 13, 2020 11:10
Copy link
Member

@bbrala bbrala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a wat to do this. Another option might be a DefaultClientFactory or something that just creates the classes with sane defaults. I mean, why wouldn't you want an different MetaParser to parse Meta information supplied by a specific JSON:API server? By seperating the factory from the classes you perhaps keep options open a little more.

If you go this route you should also change DocumentParserTest::getDocumentParser to use this?

@JaZo
Copy link
Member Author

JaZo commented Aug 14, 2020

A lot of popular packages, such as Guzzle, have a static method (instead of a factory class) to create an instance with sensible defaults, so that's why I went that route. I can also add such a factory method to the DocumentClient to even make the setup just one line.

Although the internal parsers don't have an interface, it is possible to use your own MetaParser by using the constructors. The methods introduced here are for convenience and I do not consider that as sensible defaults so I wouldn't include that in this PR.

Updating DocumentParserTest::getDocumentParser would make the tests cleaner so that is a good idea 👍

@JaZo JaZo changed the title Add factory methods for ResponseParser and DocumentParser Add factory methods for DocumentClient, ResponseParser and DocumentParser Sep 10, 2020
@JaZo
Copy link
Member Author

JaZo commented Sep 10, 2020

@bbrala I also added a factory method to DocumentClient with some basic configuration options making it even easier to use it outside Laravel. See #79 (comment) for your updated example.

@JaZo JaZo requested a review from bbrala September 10, 2020 12:12
@JaZo JaZo added the enhancement New feature or request label Sep 10, 2020
@bbrala
Copy link
Member

bbrala commented Sep 14, 2020

Looking good, but perhaps add a small section to the readme? It should be getting-started, but perhaps a little chapter somewhere is a good idea.

If that is covered, it will be approved :)

@JaZo
Copy link
Member Author

JaZo commented Sep 25, 2020

@bbrala fair point! I've updated the getting started section in the readme.

README.MD Show resolved Hide resolved
@JaZo JaZo merged commit aa0a821 into master Sep 25, 2020
@JaZo JaZo deleted the feature/parser-factories branch September 25, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants