Skip to content

Conversation

@yaziedda
Copy link

@yaziedda yaziedda commented Oct 8, 2024

Hacktoberfest

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@yaziedda Thanks for creating PR.

I can't see an enhancement in the util_curl.go; if you're interested in adding a unit test, it would be appreciated.

Can you please revert the change in util_curl.go?

@yaziedda
Copy link
Author

yaziedda commented Oct 9, 2024

@yaziedda Thanks for creating PR.

I can't see an enhancement in the util_curl.go; if you're interested in adding a unit test, it would be appreciated.

Can you please revert the change in util_curl.go?

Thanks for the feedback @jeevatkm , but for add unit test need improve util_curl.go because function buildCurlRequest not give consistent result like double space at assign domain. Example :

Expected :
curl -X GET -H 'Authorization: Bearer token' -H 'Content-Type: application/json' http://example.com

Actual from existing util_curl.go
curl -X GET -H 'Authorization: Bearer token' -H 'Content-Type: application/json' http://example.com

Screenshot from 2024-10-09 11-12-30

Left : My enhance code
Right : From existing code

jeevatkm
jeevatkm previously approved these changes Oct 9, 2024
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@yaziedda I see, thanks for the details.

@codecov
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.59%. Comparing base (bea44b7) to head (87c933d).
Report is 1 commits behind head on v2.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2     #884      +/-   ##
==========================================
+ Coverage   96.15%   96.59%   +0.43%     
==========================================
  Files          14       14              
  Lines        1821     1821              
==========================================
+ Hits         1751     1759       +8     
+ Misses         46       39       -7     
+ Partials       24       23       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jeevatkm
Copy link
Member

jeevatkm commented Oct 9, 2024

@yaziedda, I just noticed this. Can you please move the unit test to this file curl_cmd_test.go?

@yaziedda
Copy link
Author

yaziedda commented Oct 9, 2024

@yaziedda, I just noticed this. Can you please move the unit test to this file curl_cmd_test.go?

@jeevatkm I’ve already changed it to curl_cmd_test.go.

Copy link
Member

@jeevatkm jeevatkm 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 correcting it @yaziedda

@yaziedda
Copy link
Author

You're welcome, can we merge this PR @jeevatkm ?

@jeevatkm jeevatkm merged commit fc51b33 into go-resty:v2 Oct 10, 2024
5 checks passed
jeevatkm added a commit that referenced this pull request Nov 2, 2024
Co-authored-by: yaziedda <yazied_uddien@linkaja.id>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants