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

Add default client variable #73

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Add default client variable #73

merged 2 commits into from
Apr 10, 2023

Conversation

ShubhamRasal
Copy link
Contributor

  • initialize in init function
  • add helpers method to use by default client

@ShubhamRasal
Add default client variable
2b52243 
- initialize in init function
- add helpers method to use by default client
@ShubhamRasal ShubhamRasal linked an issue Mar 30, 2023 that may be closed by this pull request
Add DefaultClient and package helpers #52
Closed
@ShubhamRasal ShubhamRasal self-assigned this Mar 30, 2023
@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/codeql-analysis.yml:analyze/language:go. As part of the setup process, we have scanned this repository and found 3 existing alerts. Please check the repository Security tab to see all alerts.

@ShubhamRasal
Copy link
Contributor Author

default.go feel so generic. do you have any other name for file or location where I can move this functions?
cc @Mzack9999

@Mzack9999
Copy link
Member

@ShubhamRasal if I had to suggest something, probably I'd go with default_client.go or client.go. In the standard library the DefaultClient is under net/http/client.go

Mzack9999
Mzack9999 requested changes Mar 30, 2023
View reviewed changes
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

Could you add a test that ensure the DefaultClient is not nil and we can perform at least one operation (e.g. GET)

Mzack9999
Mzack9999 approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

lgtm!

@Mzack9999 Mzack9999 merged commit d4c5eff into main Apr 10, 2023
7 checks passed
@Mzack9999 Mzack9999 deleted the issue-52 branch April 10, 2023 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mzack9999 Mzack9999 Mzack9999 approved these changes

Assignees

@ShubhamRasal ShubhamRasal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add DefaultClient and package helpers
2 participants
@ShubhamRasal @Mzack9999