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

Fix errors with index handling; permit multiple indexes. #98

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Fix errors with index handling; permit multiple indexes. #98

wants to merge 14 commits into from

Conversation

smcmurray
Copy link

This allows options.index to be an array of indexes, prioritized by list order.

It also removes a pretty obscure error whereby

send(ctx, 'path/to/', {index: 'index', extensions:['html','htm']})

could serve path/to/index.html/index

@smcmurray
Copy link
Author

@coderhaoxin @jonathanong This PR has some tests that I think will highlight weaknesses in the current code. Even if you don't like my solution, the current code should be fixed.

@haoxins
Copy link
Member

haoxins commented Mar 23, 2018

This PR has some tests that I think will highlight weaknesses in the current code

can you split these tests code to a separate PR ~

BTW, do you really need such a big change to implement your feature ? 😢

@smcmurray
Copy link
Author

@coderhaoxin My new feature accounts for only about 1 line of that code. The rest is a bug fix.

The reality is that you currently support an open-ended number of index files, but your code doesn't account for that correctly. This PR also supports an open-ended number of index files, but accounts for them all correctly (so I say). I could alter lines 76-77 to reduce the possible permutations back to current spec. But I would leave the rest of the code.

commit 46c4c93 has the tests to expose current misbehavior. (I haven't actually tested the tests.)
commit 78e760e corrects the stupid omission of closing braces in that commit.

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.

2 participants