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.

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

Port StripFileExtensions from Desk #24

Closed
wants to merge 1 commit into from
Closed

Port StripFileExtensions from Desk #24

wants to merge 1 commit into from

Conversation

gangleri
Copy link

Copy StripFileExtensions from Desk so that it can be used in Spaces
and other projects

  • Add Sanitize middleware
  • Add tests
  • Update Gopkg.lock to include echo so tests can be run

Copy StripFileExtensions from Desk so that it can be used in Spaces
and other projects

- Add Sanitize middleware
- Add tests
- Update Gopkg.lock to include echo so tests can be run
@codecov
Copy link

codecov bot commented Apr 17, 2018

Codecov Report

Merging #24 into master will decrease coverage by 0.19%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #24     +/-   ##
=========================================
- Coverage   59.78%   59.59%   -0.2%     
=========================================
  Files          11       12      +1     
  Lines         378      396     +18     
=========================================
+ Hits          226      236     +10     
- Misses        127      132      +5     
- Partials       25       28      +3
Impacted Files Coverage Δ
sanitizeMiddleware/sanitize.go 55.55% <55.55%> (ø)

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 2d01d64...7cb9889. Read the comment docs.

@arp242
Copy link
Contributor

arp242 commented Apr 17, 2018

I don't think we should port this to be honest. It's an ugly hack. I have never happy with it in Desk either, and the only reason we added it in Desk was because we were supposed to move away from adding .json to all our endpoints.

It would be better to either simply not use .json (easiest), or fix echo's router or swap it for something else (harder).

@ready4god2513
Copy link
Contributor

We don't get the choice to not use .json. It's something that's been decided on. So now we need to support it everywhere.

@arp242
Copy link
Contributor

arp242 commented Apr 17, 2018

Then someone should fix the echo router. Adding middlewares is very annoying because it adds two entries to the stack trace on any error. It's already almost two pages in a full-screen terminal on my screen. Every time I have an error I need to scroll up. Very annoying when trying to fix stuff.

The only reason my PR got reverted is because it had a bug, so if someone fixes that, then we can just use the echo router.

@ready4god2513
Copy link
Contributor

ready4god2513 commented Apr 17, 2018 via email

@@ -28,6 +28,39 @@
packages = ["."]
revision = "7cafcd837844e784b526369c9bce262804aebc60"

[[projects]]
name = "github.com/labstack/echo"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in echoutil btw, as we use this package in some projects that don't use echo, and this will add echo as a dependency.

@gangleri
Copy link
Author

Closing this PR as it's been replaced by https://github.com/Teamwork/echoutil/pull/11

@gangleri gangleri closed this Apr 18, 2018
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.

None yet

3 participants