pillarjs
Return redirection for symlinks
Hi! Thanks for the pull request! Please undo all the style changes, mainly because they are unwelcome (we weren't asking for style changes) and it makes it really hard for me to understand what changed from looking at the diff.
Also, please add documentation and tests for the new feature so we can accept it :)
Hi! Your original suggested I agreeded to was "Add an option to show the symbolic links as 301 redirections to the file location instead of return the file content.". I don't see an option to enable the feature. If this always enabled? If not, what is the option? We need an option to enable/disable this feature. If you want it accepted any time soon, you should probably default it to disabled so it's backwards compatible.
Oh, sorry, looks like you added a test from when I looked at it hours ago, haha. Please try to add more tests, including non happy paths. A new feature should have many tests, not just one, as there are probably edge cases you need to test for, for example, what about folder junctions vs folder symlinks? I wouldn't expect a junction to redirect, is that the case with these changes or no?
Hi @piranna, sorry, I have not taken a look since you changed the pull request. Can you at least take a look and fix the failing tests so the pull request passes the test while I take a look at the new changes? Also note that one of the tests is marked as "double callback!" with your changes, so even though that's not causing a failure, you'll need to get that fixed as well.
Tests are failling to some unrelated test I can't be able to identify what's the source of the problem ("should return 500 on error when reading file"). Could you be able to take a look so this could be merged?
The problem that's giving is about the callback being called twice, probably due to the nextTick() call in the test.
There are still a lot of failing tests and you haven't fixed the double callback, either. In addition, it looks like when you merged, you accidentally reverted code, including a security fix. Please fix all that up and ping me and I'll come back and take a look :)!
I am going to close this. It seems like a good idea but we would need to have a much more clear and clean change set to ever merge this, which seems unlikely at this point. Feel free to reopen a new PR with just the minimal changes required to land the feature if you are willing and able to work on this again in the future.
