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

EPERM and EBUSY errors #46

Closed

Conversation

rafaelcastrocouto
Copy link

EPERM and EBUSY now passing as null, not error in function stat

EPERM and EBUSY now passing as null, not error in function stat
@dougwilson
Copy link
Contributor

Hi! I'm not sure if I understand the purpose of the change. Can you provide the reason(s) why you think those errors should map to a HTTP 404 vs another error code? Also, please add tests to the pull request.

@rafaelcastrocouto
Copy link
Author

I can't tell exactly why, but some files on my root direcrtory (c:/) were preventing serve-index to show the root's files and foldes. Instead I was receiving only the error messages as plain text.
With this change it shows everything just as they should be.
It worked for me, so I thought I should share!

@dougwilson
Copy link
Contributor

Oh, haha, that makes sense, actually. I for some reason read this repo's name as serve-static. If you can add some tests that would help us get this merged in quicker :)

@rafaelcastrocouto
Copy link
Author

I'm sorry but could you point me some direction on what kind of tests I should add? I have no idea on how to reproduce the EPERM and EBUSY errors with mocha.

@dougwilson
Copy link
Contributor

Hey! I'll have to take a look in a few days when I have some time. The reason we need tests is that 1) the PR is actually failing because the code coverage is reduced from your change and 2) it helps insure that someone does not come along and break this feature you worked to get added :)

@dougwilson
Copy link
Contributor

I never really had time to look and with this dead for years now, I'm just going to close since there are no tests and it doesn't pass the CI checks to be able to merge. You're always welcome to open a new PR with tests 👍

@dougwilson dougwilson closed this Aug 11, 2018
@rafaelcastrocouto
Copy link
Author

Sure, no problem at all.
Maybe in the future this can be added as an "advanced error handling" option.
Or not ¯_(ツ)_/¯

@dougwilson
Copy link
Contributor

Yea, that's another possibility as well. You can also open an issue with the steps to reproduce the issue and myself or otherwise can take a look into forming a PR that resolves the underlying issue with tests. Lots of possibilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants