Skip to content

Conversation

@RashadAnsari
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #1706 (0406abe) into master (b90e4e8) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1706      +/-   ##
==========================================
+ Coverage   84.88%   84.98%   +0.10%     
==========================================
  Files          29       29              
  Lines        1945     1958      +13     
==========================================
+ Hits         1651     1664      +13     
  Misses        187      187              
  Partials      107      107              
Impacted Files Coverage Δ
response.go 85.18% <100.00%> (ø)
middleware/jwt.go 77.89% <0.00%> (+1.75%) ⬆️
middleware/static.go 68.65% <0.00%> (+3.08%) ⬆️

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 b90e4e8...0406abe. Read the comment docs.

@iambenkay
Copy link
Contributor

iambenkay commented Dec 2, 2020

Any reason for this change?
If you could link an issue or something

@RashadAnsari
Copy link
Contributor Author

Any reason for this change?
If you could link an issue or something

Hi @iambenkay

We need to change the status code of a request in some situations using the middleware.

For example:

e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
	return func(c echo.Context) error {
		c.Response().Before(func() {
			if 200 < c.Response().Status && c.Response().Status < 300 {
				c.Response().Status = 200
			}
		})

		return next(c)
	}
})

@iambenkay
Copy link
Contributor

okay I understand. so you need it set before the beforeFuncs are called.
makes sense. Nice one.

@iambenkay
Copy link
Contributor

@lammel

@lammel
Copy link
Contributor

lammel commented Dec 6, 2020

@RashadAnsari If we make this an intended behaviour, we should a test for it too, to ensure it stays that way.
Could you a test (like in your example with mapping status 2xx to 200) for it too please.

I will merge then.

@RashadAnsari
Copy link
Contributor Author

RashadAnsari commented Dec 6, 2020

@RashadAnsari If we make this an intended behaviour, we should a test for it too, to ensure it stays that way.
Could you a test (like in your example with mapping status 2xx to 200) for it too please.

I will merge then.

@lammel
Done!

@lammel
Copy link
Contributor

lammel commented Dec 6, 2020

Thanks. That was fast ;-)
Merging.

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.

3 participants