Replace http constants with stdlib ones

[ribice]

Resolves #1190

[vishr]

Wouldn't it break code for users? Perhaps we can mark it deprecated and drop it in a future release.

[ribice]

If doubt anyone uses echo.Get instead of http.MethodGet.

Also it's quite easy to replace it if it becomes breaking change for someone.

This change simplifies the codebase by reusing constants from stdlib.

[nipemgroup]

Indeed it's very quick to fix, but it's used in the documentation; https://echo.labstack.com/middleware/cors.
I used those constants, rather than the stdlib ones, because of the docs. I know it's a little late, but I can't be the only one getting broken code from this change.

[vishr]

@nipemgroup I have updated the docs to reflect this change.

[alexaandru]

I missed the fact that they are in the docs when I reviewed this, @vishr was right to bet on backward compatibility. I pushed a PR to address this: #1209 - adding the exported constants back.

I propose we 1st change the documentation, then we give proper notice, and then (maybe?) we remove them later.

[vishr]

Standard constants for methods came recently so there may be users using it. I am okay to replace it internally but let’s keep constants if just in case they people are using it.

…

Sent from my iPhone

On Oct 13, 2018, at 1:05 PM, Emir Ribić ***@***.***> wrote: @ribice commented on this pull request. In echo.go: > @@ -128,20 +128,6 @@ type ( } ) -// HTTP methods If doubt anyone uses echo.Get instead of http.MethodGet. Also it's quite easy to replace it if it becomes breaking change for someone. This change simplifies the codebase by reusing constants from stdlib. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[alexaandru]

I like this change, and while I value greatly backward compatibility, I also believe there is very little reason why people would use these directly. They use e.GET() & friends instead. Also, a quick search shows 0 projects using this on Github: https://github.com/search?utf8=%E2%9C%93&q=echo.Get+language%3AGo&type=Repositories&ref=advsearch&l=Go&l=

Add to that, that for production one should vendor their dependencies anyway & use tests, etc. so the chances of doing real damage with this become very small.

I would just suggest to change the commit message to give a little more info by itself, so that someone would know how to fix things if they break, from the commit message alone (though the diff itself is self explanatory):

Replace http constants with stdlib ones, i.e.: http.MethodGet instead of echo.GET, etc.

[vishr]

@alexaandru Sounds good, @ribice do you want to fix the commit message?

[ribice]

Will do tomorrow.

[ribice]

Done, also fixed conflicts.

[vishr]

@ribice Can you fix the build? Looks like cors_test.go still uses old constants.

[codecov]

Codecov Report

Merging #1205 into master will not change coverage.
The diff coverage is 97.61%.

@@           Coverage Diff           @@
##           master    #1205   +/-   ##
=======================================
  Coverage   81.61%   81.61%           
=======================================
  Files          27       27           
  Lines        1931     1931           
=======================================
  Hits         1576     1576           
  Misses        248      248           
  Partials      107      107

Impacted Files	Coverage Δ
middleware/proxy_1_11.go	100% <100%> (ø)	⬆️
group.go	93.75% <100%> (ø)	⬆️
router.go	93.3% <100%> (ø)	⬆️
middleware/csrf.go	76.71% <100%> (ø)	⬆️
middleware/method_override.go	84.61% <100%> (ø)	⬆️
bind.go	86.3% <100%> (ø)	⬆️
middleware/cors.go	75% <100%> (ø)	⬆️
echo.go	87.72% <90%> (ø)	⬆️

---

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 059c099...52bd56c. Read the comment docs.

[ribice]

@ribice Can you fix the build? Looks like cors_test.go still uses old constants.

Fixed.