Skip to content
This repository was archived by the owner on Apr 14, 2024. It is now read-only.

Conversation

rodnaph
Copy link
Contributor

@rodnaph rodnaph commented Dec 28, 2020

Summary

Adds an interface GotenbergClientInterface, implemented by the Client class.

The purpose is to aid testing in other applications so the client class can be easily mocked, as the current Client class is final which makes it tricky to simulate in unit tests. Currently when working with tools that do not allow mocking final classes you need to instantiate the client class and mock the http client and internal interactions with the request/response classes, as well as file writes.

Test plan (required)

I've not added any tests as this is a non-functional change - we could test the interface is implemented though if you think it's worth it?

Checklist

  • Have you followed the guidelines in our CONTRIBUTING guide?
  • Have you lint your code locally prior to submission (make lint)?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally (make tests)?
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code

@rodnaph
Copy link
Contributor Author

rodnaph commented Dec 28, 2020

@gulien Unsure what's going on with the build, if you can give me some tips I can look at it.

@gulien
Copy link
Contributor

gulien commented Dec 28, 2020

Hello @rodnaph

Thanks for your PR! 👍

The package thecodingmachine/safe seems to only work with PHP 7.2, while our tests run with PHP 7.1.

I guess we should remove safe as it does not make sense in a library and may cause conflicts with an application using it too (version conflicts).

@rodnaph
Copy link
Contributor Author

rodnaph commented Dec 28, 2020

@gulien Ok thanks - I'll raise removing Safe as a separate PR 👍

@rodnaph rodnaph mentioned this pull request Dec 28, 2020
6 tasks
@rodnaph
Copy link
Contributor Author

rodnaph commented Dec 30, 2020

@gulien Rebased on master now build is passing, looks to be running duplicate builds for pull_request and pull_request_target though, I don't really understand if both of these are required though...

I think pull_request_target should be removed...

@gulien
Copy link
Contributor

gulien commented Dec 30, 2020

No idea 😄

RTM?

@rodnaph
Copy link
Contributor Author

rodnaph commented Dec 30, 2020

I did RTM, still unclear... I've removed it.

@rodnaph
Copy link
Contributor Author

rodnaph commented Jan 4, 2021

@gulien Any changes needed on this one? I think you need to remove the codecov/travisci checks from this repo in the settings btw (can be codified by probot if needed I think).

@gulien
Copy link
Contributor

gulien commented Jan 4, 2021

Ok for me 👍 thanks again @rodnaph

@gulien gulien merged commit 7bfaaa7 into thecodingmachine:master Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants