Deprecate usage of white-space character in path

[PradnyaBaviskar]

Referring to expressjs/express#2511, added fix to deprecate usage of white-space character in path.

Can one of the maintainers please review this?

[coveralls]

Coverage decreased (-86.87%) to 13.13% when pulling 4bfb984 on PradnyaBaviskar:express-issue-2511 into a76d908 on pillarjs:master.

[coveralls]

Coverage decreased (-86.87%) to 13.13% when pulling 4bfb984 on PradnyaBaviskar:express-issue-2511 into a76d908 on pillarjs:master.

[coveralls]

Coverage decreased (-1.01%) to 98.99% when pulling dfa6a62 on PradnyaBaviskar:express-issue-2511 into a76d908 on pillarjs:master.

[blakeembrey]

I'll review this in the morning now, but we should make sure it's being tested 👍 It brings up another interesting conversation around path matching too.

[bajtos]

@blakeembrey we have decided with @PradnyaBaviskar to omit the tests because I was not aware of any easy way how to write them. Can you advise please? Should we hook into process.on('deprecation', fn) to check that the deprecation message was reported?

[blakeembrey]

@bajtos You're right, it would be a little fragile to test but we should make sure it is working and doesn't go missing one day. Maybe override process.stderr for the test case and restore it after? Or you could listen for events and use that to end the test (seems cleaner to me)? Either way, it may not end up being a deprecation depending on what happens in #42

[bajtos]

Maybe override process.stderr for the test case and restore it after?

-1, I'd rather not mess with global state that may be already overridden by mocha test runner.

Or you could listen for events and use that to end the test (seems cleaner to me)?

I think this is the correct way to go. Listen for a deprecation event, set a flag when it's called, and then assert in the test that after the tested method was invoked, the flag is set.

Either way, it may not end up being a deprecation depending on what happens in #42

What do you propose? Should @PradnyaBaviskar continue working on this patch, or shall we close it in favour of whatever get's implemented as part of #42?

[blakeembrey]

@bajtos I actually think this isn't something we want right now. Maybe at the framework level (Express) it makes sense, but it's entirely possible that what is being put into the RegExp is already decoded (outside of Express, but in other frameworks, etc). If you disagree, feel free to let me know - but I'm going to close this for now.