Skip to content

Conversation

@linxGnu
Copy link
Contributor

@linxGnu linxGnu commented Apr 25, 2019

This PR makes changes:

  • The sdk is not responsible for handling context (in each http request) error. Therefore, following code will be removed:
                res, err := client.httpClient.Do(req.WithContext(ctx))
		if err != nil {
			select {
			case <-ctx.Done():
				err = ctx.Err()
			default:
			}
		}
  • Using constant defined in http std package instead of hard code. In this way:
    req, err := http.NewRequest("GET", client.url(endpoint), nil) will be rewritten as
    req, err := http.NewRequest(http.MethodGet, client.url(endpoint), nil)

  • Update test along with changes

@linxGnu linxGnu requested review from mokejp and sugyan April 25, 2019 09:33
@CLAassistant
Copy link

CLAassistant commented Apr 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Apr 25, 2019

Codecov Report

Merging #150 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
- Coverage   72.11%   72.07%   -0.04%     
==========================================
  Files          23       23              
  Lines        1757     1751       -6     
==========================================
- Hits         1267     1262       -5     
+ Misses        413      412       -1     
  Partials       77       77
Impacted Files Coverage Δ
linebot/client.go 73.52% <100%> (-0.8%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e039956...05d0725. Read the comment docs.

@nasa9084
Copy link
Member

LGTM

@tkgauri tkgauri requested review from nasa9084 and removed request for mokejp and sugyan May 28, 2019 08:01
@nasa9084 nasa9084 merged commit 49409e9 into line:master May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants