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

Improvements and handle Google errors #10

Merged
merged 3 commits into from
Apr 14, 2016
Merged

Improvements and handle Google errors #10

merged 3 commits into from
Apr 14, 2016

Conversation

rgazelot
Copy link
Contributor

@rgazelot rgazelot commented Apr 6, 2016

throw new Exception\BackendException;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using a trait instead ?

@Taluu
Copy link
Contributor

Taluu commented Apr 6, 2016

Instead of AbstractApi, you should use a trait, and the exceptions are quite related to google point of view IMO...

And the handleErrors isn't pretty imo, it should be triggered only on cases where the exception code is not what we expect (not a 200 when we should exepct a 200 ? call that method. Not a 201 ? etc, etc)

@rgazelot
Copy link
Contributor Author

rgazelot commented Apr 6, 2016

the exceptions are quite related to google point of view IMO

Fortunately, we are in the Google adapter...

@rgazelot
Copy link
Contributor Author

rgazelot commented Apr 6, 2016

@Taluu For the second point, maybe handleErrors is a bad name, this is more an analyzeResponse

@Taluu Taluu changed the title Improvements and handle Google errors [WIP] Improvements and handle Google errors Apr 6, 2016
@rgazelot
Copy link
Contributor Author

rgazelot commented Apr 7, 2016

@Taluu Open for review. (I'll rebase and change the name of some commits :p)

@@ -33,6 +28,8 @@
*/
class CalendarApi implements CalendarApiInterface, AclInterface
{
use ResponseHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@rgazelot rgazelot force-pushed the imp-errors branch 2 times, most recently from f51ae1a to 82d3644 Compare April 8, 2016 09:49
@rgazelot
Copy link
Contributor Author

rgazelot commented Apr 8, 2016

@Taluu I think i'm done with this one.

[500, 'Backend Error', Exception\BackendException::class],
];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of making a blank mock Api class, what you can do is using a mock that use the trait with $this->getMockForTrait() : https://phpunit.de/manual/current/en/test-doubles.html#test-doubles.mocking-traits-and-abstract-classes

@Taluu
Copy link
Contributor

Taluu commented Apr 8, 2016

Please keep in mind we're on php 5.4 >, not php 5.5 (so no class...).
I should raise the requirement, but this is not the scope of this PR IMO (or 1.x actually...)

@Taluu
Copy link
Contributor

Taluu commented Apr 8, 2016

This looks good though, so just replace the ::class with the full qualified names and it's good to go

@rgazelot
Copy link
Contributor Author

rgazelot commented Apr 8, 2016

@Taluu done

@Taluu Taluu changed the title [WIP] Improvements and handle Google errors Improvements and handle Google errors Apr 8, 2016
@rgazelot
Copy link
Contributor Author

@Taluu I fixed the php 5.4 stuffs

@Taluu
Copy link
Contributor

Taluu commented Apr 14, 2016

🎉

@Taluu Taluu merged commit 1948c1c into master Apr 14, 2016
@Taluu Taluu deleted the imp-errors branch April 14, 2016 08:54
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

2 participants