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

feat: Export transport method to return current transport from the client #605

Merged
merged 1 commit into from Mar 21, 2023
Merged

feat: Export transport method to return current transport from the client #605

merged 1 commit into from Mar 21, 2023

Conversation

MicBun
Copy link
Contributor

@MicBun MicBun commented Nov 29, 2022

Fixes #601

@MicBun MicBun requested a review from xD-saleem January 18, 2023 10:07
xD-saleem
xD-saleem previously approved these changes Feb 2, 2023
View reviewed changes
@MicBun
Copy link
Contributor Author

MicBun commented Feb 2, 2023

Can this be merged or closed? I don't have write access

segevda
segevda reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@segevda segevda left a comment

Choose a reason for hiding this comment

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

Most of the lines changed in this commit are fixing the indentation. IMO you should remove these changes from this commit as they are unrelated. Another PR can be created to fix the indentation.

@MicBun MicBun requested a review from segevda March 1, 2023 16:23
@MicBun
Copy link
Contributor Author

MicBun commented Mar 1, 2023

Most of the lines changed in this commit are fixing the indentation. IMO you should remove these changes from this commit as they are unrelated. Another PR can be created to fix the indentation.

thank you for the input

segev-indegy

This comment was marked as outdated.

Sign in to view

@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2023

@MicBun Thanks for your PR and contribution. I'm sorry for the delayed attention on the PR.

May I know the context of the PR! I couldn't understand, why its needed.

@MicBun
Copy link
Contributor Author

MicBun commented Mar 6, 2023

@MicBun Thanks for your PR and contribution. I'm sorry for the delayed attention on the PR.

May I know the context of the PR! I couldn't understand, why its needed.

I am trying to resolve this issue #601 here
by changing transport into Transport to make it exported

@jeevatkm jeevatkm added this to the v2.8.0 Milestone milestone Mar 19, 2023
@jeevatkm jeevatkm changed the title Fix export transport based on doc comment Export transport method to return current transport from the client Mar 19, 2023
@jeevatkm jeevatkm changed the title Export transport method to return current transport from the client feat: Export transport method to return current transport from the client Mar 19, 2023
@codecov
Copy link

codecov bot commented Mar 19, 2023 •
edited

Codecov Report

Merging #605 (d4774c6) into master (38b1644) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #605   +/-   ##
=======================================
  Coverage   95.83%   95.83%           
=======================================
  Files          11       11           
  Lines        1559     1559           
=======================================
  Hits         1494     1494           
  Misses         40       40           
  Partials       25       25
Impacted Files Coverage Δ
client.go 97.96% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jeevatkm
jeevatkm approved these changes Mar 19, 2023
View reviewed changes
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.

@MicBun Thanks for your contribution.

jeevatkm
jeevatkm requested changes Mar 19, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment •
edited

Choose a reason for hiding this comment

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

@MicBun I just noticed, this PR contains unnecessary commits. Can you please update your PR with only your changes or create a new?
After that we can merge this one.

@MicBun
Copy link
Contributor Author

MicBun commented Mar 20, 2023

@MicBun I just noticed, this PR contains unnecessary commits. Can you please update your PR with only your changes or create a new? After that we can merge this one.

sure, thank you for the feedback

@loris007
Copy link

loris007 commented Mar 20, 2023 •
edited

fyi: the transport is already accessible via http.Client

client := resty.New()
transport := client.GetClient().Transport

@MicBun MicBun requested review from jeevatkm and removed request for segevda March 21, 2023 08:02
jeevatkm
jeevatkm approved these changes Mar 21, 2023
View reviewed changes
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 @MicBun

@jeevatkm jeevatkm merged commit cf2d4c4 into go-resty:master Mar 21, 2023
4 checks passed
@MicBun MicBun deleted the export-transport branch March 21, 2023 23:57
@MicBun
Copy link
Contributor Author

MicBun commented Mar 21, 2023

Thanks @MicBun

@jeevatkm You're welcome! I'm glad that I could contribute and be of help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@segevda segevda segevda left review comments

@jeevatkm jeevatkm jeevatkm approved these changes

@xD-saleem xD-saleem xD-saleem left review comments

@segev-indegy segev-indegy segev-indegy left review comments

Assignees

@MicBun MicBun

Labels
enhancement
Milestone
v2.8.0 Milestone
Development

Successfully merging this pull request may close these issues.

client Transport is not exported
6 participants
@MicBun @jeevatkm @loris007 @segevda @xD-saleem @segev-indegy