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

PHP - Request Adapter #1048

Merged
merged 15 commits into from
Jan 28, 2022
Merged

PHP - Request Adapter #1048

merged 15 commits into from
Jan 28, 2022

Conversation

Ndiritu
Copy link
Contributor

@Ndiritu Ndiritu commented Jan 21, 2022

Closes #364

@Ndiritu Ndiritu added the PHP label Jan 21, 2022
@Ndiritu Ndiritu marked this pull request as draft January 21, 2022 14:22
@Ndiritu Ndiritu force-pushed the php/request-adapter-impl branch from a0215c6 to 379448d Compare January 21, 2022 14:34
@Ndiritu Ndiritu force-pushed the php/request-adapter-impl branch from 379448d to 00dcbc6 Compare January 21, 2022 14:40
@Ndiritu Ndiritu force-pushed the php/request-adapter-impl branch from b29ac1f to db3bca7 Compare January 21, 2022 14:58
@Ndiritu Ndiritu marked this pull request as ready for review January 21, 2022 15:01
@Ndiritu Ndiritu requested a review from SilasKenneth January 21, 2022 15:13
baywet
baywet requested changes Jan 21, 2022
View reviewed changes
Copy link
Member

@baywet baywet 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 putting all of this together so fast! A few minor comments.

http/php/guzzle/src/GuzzleRequestAdapter.php Show resolved Hide resolved
@baywet baywet added this to the Community Preview milestone Jan 21, 2022
@baywet
Copy link
Member

baywet commented Jan 21, 2022

Also, can you add an entry to the changelog please?

SilasKenneth
SilasKenneth requested changes Jan 21, 2022
View reviewed changes
http/php/guzzle/src/GuzzleRequestAdapter.php Outdated Show resolved Hide resolved
http/php/guzzle/src/KiotaClientFactory.php Outdated Show resolved Hide resolved
http/php/guzzle/src/KiotaClientFactory.php Outdated Show resolved Hide resolved
http/php/guzzle/src/KiotaClientFactory.php Outdated Show resolved Hide resolved
http/php/guzzle/src/KiotaClientFactory.php Outdated Show resolved Hide resolved
http/php/guzzle/tests/GuzzleRequestAdapterTest.php Show resolved Hide resolved
abstractions/php/src/RequestAdapter.php Show resolved Hide resolved
http/php/guzzle/src/GuzzleRequestAdapter.php Outdated Show resolved Hide resolved
@Ndiritu Ndiritu marked this pull request as draft January 25, 2022 09:03
@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 25, 2022 •
edited
Loading

Also, can you add an entry to the changelog please?

@baywet Added

@Ndiritu Ndiritu force-pushed the php/request-adapter-impl branch from 2fbb043 to 04f09a1 Compare January 25, 2022 13:20
@Ndiritu Ndiritu marked this pull request as ready for review January 25, 2022 13:25
baywet
baywet requested changes Jan 26, 2022
View reviewed changes
Copy link
Member

@baywet baywet 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 in the changes, just one minor comment left.
Also, is the Async suffix idiomatic to PHP? don't hesitate to drop it if not.

abstractions/php/src/Types/Date.php Outdated Show resolved Hide resolved
abstractions/php/src/Types/Time.php Outdated Show resolved Hide resolved
@Ndiritu Ndiritu force-pushed the php/request-adapter-impl branch from a50de02 to 48659f1 Compare January 27, 2022 09:33
@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 27, 2022

thanks for taking in the changes, just one minor comment left. Also, is the Async suffix idiomatic to PHP? don't hesitate to drop it if not.

On the "Async" suffix, I haven't found any thoughts explicitly against it. The Guzzle library uses it in some methods. I'd be open to changing this based on community feedback.

baywet
baywet requested changes Jan 27, 2022
View reviewed changes
Copy link
Member

@baywet baywet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

On the "Async" suffix, I haven't found any thoughts explicitly against it. The Guzzle library uses it in some methods. I'd be open to changing this based on community feedback.

perfect, let's see what feedback we get during the preview.

One last thing, please update the table on the readme

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 27, 2022

On the "Async" suffix, I haven't found any thoughts explicitly against it. The Guzzle library uses it in some methods. I'd be open to changing this based on community feedback.

perfect, let's see what feedback we get during the preview.

One last thing, please update the table on the readme

Updated

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jan 27, 2022 •
edited
Loading

@SilasKenneth requesting your further review/sign-off on this PR

@baywet baywet requested a review from SilasKenneth January 27, 2022 20:41
@SilasKenneth SilasKenneth merged commit 727026a into main Jan 28, 2022
@SilasKenneth SilasKenneth deleted the php/request-adapter-impl branch January 28, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baywet baywet baywet approved these changes

@SilasKenneth SilasKenneth SilasKenneth approved these changes

@andrueastman andrueastman Awaiting requested review from andrueastman

@darrelmiller darrelmiller Awaiting requested review from darrelmiller

@ddyett ddyett Awaiting requested review from ddyett

@MIchaelMainer MIchaelMainer Awaiting requested review from MIchaelMainer

@nikithauc nikithauc Awaiting requested review from nikithauc

@peombwa peombwa Awaiting requested review from peombwa

@ramsessanchez ramsessanchez Awaiting requested review from ramsessanchez

@zengin zengin Awaiting requested review from zengin

Assignees

@Ndiritu Ndiritu

Labels
PHP
Projects
None yet
Milestone
Community Preview
Development

Successfully merging this pull request may close these issues.

Implement Request Adapter
3 participants
@Ndiritu @baywet @SilasKenneth