Skip to content

Conversation

@Art4
Copy link
Collaborator

@Art4 Art4 commented Mar 30, 2021

Goal of this PR is a new NativeCurlClient like the existing one, that will work native with curl without other dependencies. I'm still at the work figuring out the necessary public method that will be needed. In the next major release this client should replace the existing Redmine\Client.

In the best case every curl options could be managed with setCurlOption() and unsetCurlOption(), but now I have to create a way to manage the possible setted headers.

@Art4 Art4 added enhancement pending: help wanted help and PRs are welcome labels Mar 30, 2021
@Art4 Art4 self-assigned this Mar 30, 2021
@Art4 Art4 marked this pull request as draft March 30, 2021 07:55
@Art4 Art4 marked this pull request as ready for review April 5, 2021 21:10
@Art4 Art4 requested a review from kbsali April 5, 2021 21:10
@Art4
Copy link
Collaborator Author

Art4 commented Apr 5, 2021

Hey @kbsali this PR is ready for review. I've implemented nearly all existing features from the old client in the new NativeCurlClient. I've created the tests for curl by overriding the native curl functions. This way I could avoid using a MockClient for tests.

I've also created a migration doc and changed the README to not mention the old client anymore.

@Art4 Art4 changed the title [WIP] Add new NativeCurlClient Add new NativeCurlClient Apr 5, 2021
@Art4 Art4 removed the pending: help wanted help and PRs are welcome label Apr 6, 2021
Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

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

amazing job! :)
👍

Art4 and others added 2 commits April 10, 2021 23:13
Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

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

thanks for taking the time to apply some of my comments! :) 👍

@Art4
Copy link
Collaborator Author

Art4 commented Apr 11, 2021

Can I resolve all open conversations and merge this PR?

@kbsali
Copy link
Owner

kbsali commented Apr 11, 2021

please do yes! :)

@Art4 Art4 merged commit c5f19cd into kbsali:master Apr 11, 2021
@Art4 Art4 deleted the add-nativecurlclient branch April 11, 2021 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants