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

Create v2.0 of the library #34

Closed
wants to merge 3 commits into from
Closed

Create v2.0 of the library #34

wants to merge 3 commits into from

Conversation

typhonius
Copy link
Owner

…oint classes.

@coveralls
Copy link

coveralls commented Nov 29, 2019

Coverage Status

Coverage increased (+1.7%) to 98.886% when pulling ca41b97 on 2.0 into 23fcc10 on master.

@typhonius
Copy link
Owner Author

typhonius commented Dec 2, 2019

@eporama @q0rban @timmillwood I know that you have been fairly active in this library. It'd be great to get some views from yourselves on this approach as it attempts to reduce the amount of code in each class (rather than having one mega client class we can use individual ones for each endpoint).

This would be a completely API breaking change, so if merged, I'd be cutting a 2.0 release for this.

@eporama
Copy link
Contributor

eporama commented Dec 5, 2019

I'm all for this approach as I drastically prefer being able to say things about the objects instead of just the client.

I am curious if it would be beneficial to go one level deeper and define the classes based on the objects instead of just on the endpoints.

example: instead of just a Teams class, we'd have a Team class with a lightweight Teams collection.

I think something like:

$team = new Team($client, $uuid);
echo "Deleting team: $team->getName()" . PHP_EOL;
$team->delete();

or

$myTeams = new Teams($client);
echo "my teams: " . PHP_EOL;
foreach ($myTeams as $team) {
  echo $team->getName() . PHP_EOL;
}

@typhonius
Copy link
Owner Author

I've thought about breaking it down further, especially since environments has so many endpoints that it really shouldn't just be one class. An example of this might be to split out things like crons and SSL certificates into their own top level class which then either translates to an environment endpoint or extends the Environments class.

That said, I'm totally unsure of whether this would be a best practice or would couple things too closely.

$certificates = new Certificates($client, $environmentUuid);
/* @var $certificate Certificate */
foreach ($certificates as $certificate) {
  echo $certificate->delete() . PHP_EOL;
}

I quite like what you've proposed as it seems clean and simple to use as a library consumer, do you know off the top of your head if this would impact testing/dependency injection at all?

@typhonius
Copy link
Owner Author

typhonius commented Dec 7, 2019

After just adding the notification class, I think it doesn't make sense to align to Acquia's endpoints with classes. The latest addition shows that when we're getting one notification vs all, we have to load different classes.

Getting a single notification

$notifications = new Notifications($client);
$result = $notifications->get('f4b37e3c-1g96-4ed4-ad20-3081fe0f9545');

Getting all notifications

$applications = new Applications($client);
$result = $applications->getNotifications('aa414bec-1fe0f-9545-813b-c15aa572ef11');

To me, it would be more logical to have one Notifications class and get and getAll methods. We can then extend this further with Crons, Certificates etc.

Potential method

$notifications = new Notifications($client);
// Load using a notification UUID
$single = $notifications->get('f4b37e3c-1g96-4ed4-ad20-3081fe0f9545');
// Load using an application UUID
$all = $notifications->getAll('aa414bec-1fe0f-9545-813b-c15aa572ef11');

We could potentially also implement a CloudApi interface with common methods (get, getAll, create, delete etc)

@typhonius
Copy link
Owner Author

typhonius commented Dec 8, 2019

This still needs more work - @eporama does this look on the right track to you? One I'm not sure where to put would be the code tasks. I've put deploy/switch/getBranches into their own class but they could also probably live in Environments.

@typhonius typhonius changed the title Restructures library to break down the Client class into logical endp… Restructures library into logical classes Dec 9, 2019
@typhonius typhonius changed the title Restructures library into logical classes Create v2.0 of the library Dec 25, 2019
@typhonius
Copy link
Owner Author

This has now been merged back into master.

@typhonius typhonius deleted the 2.0 branch October 17, 2020 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants