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.

Sign up for GitHub

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

Jump to bottom

Setle API Update #1

Merged
merged 12 commits into from
Mar 6, 2023
Merged

Setle API Update #1

merged 12 commits into from
Mar 6, 2023

Conversation

marsnys
Copy link
Contributor

@marsnys marsnys commented Feb 20, 2023

No description provided.

DeKerpelLaurent
DeKerpelLaurent reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@DeKerpelLaurent DeKerpelLaurent left a comment

Choose a reason for hiding this comment

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

Directly to main ?

tests/SetleTest.php
@@ -19,20 +19,20 @@ class SetleTest extends ApiTestCase
{
public function testBrokerToken(): void
{
$api = new Setle('foo');
$api = new Setle('foo', 'bar');
Copy link
Contributor

Choose a reason for hiding this comment

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

foo bar 😅

@fyrts
Copy link
Member

fyrts commented Feb 22, 2023

Functioneel lijkt het te werken, maar volgende puntjes zou ik wel zeker beetnemen:

  • Tests falen momenteel. Die moeten terug in orde gebracht worden.
  • Ik zou deze niet rechtstreeks naar main pushen.
  • PHPCS en PHPStan moeten nog toegepast worden.
  • De response time van Setle is wat aan de onvoorspelbare kant. Ik zou de default timeout misschien verhogen naar 30 sec.

fyrts
fyrts reviewed Feb 23, 2023
View reviewed changes
composer.json Outdated Show resolved Hide resolved
fyrts
fyrts reviewed Feb 23, 2023
View reviewed changes
composer.json Show resolved Hide resolved
fyrts
fyrts reviewed Feb 23, 2023
View reviewed changes
src/Request/Request.php Outdated Show resolved Hide resolved
DeKerpelLaurent
DeKerpelLaurent reviewed Mar 6, 2023
View reviewed changes
src/Exception/ApiException.php Outdated Show resolved Hide resolved
src/Exception/AuthException.php Outdated Show resolved Hide resolved
@DeKerpelLaurent DeKerpelLaurent merged commit e57d25a into main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fyrts fyrts fyrts left review comments

@DeKerpelLaurent DeKerpelLaurent DeKerpelLaurent approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marsnys @fyrts @DeKerpelLaurent