-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add auto extensions #33
Conversation
I like this! Plenty of (production) web servers allow this set-up, and it makes sense for |
I think this is ready to go. (We’ll see what CI says!) A few potentially controversial things that might require discussion:
|
I think I’d prefer not to have this. The chance of it being too confusing is too high. Ruby does have
This is a good change! I’ve extracted the changes (without the auto_extensions) into its own standalone PR (#34, already merged).
A better approach (which I’ve used) is to use % bundle exec adsf/bin/adsf --help
Usage: adsf [options]
[snip]
% cd adsf
% bundle exec bin/adsf --help
Usage: adsf [options]
[snip] As far as I know, the |
Looks good (and mergeable) apart from:
|
One more thing: can you add a test that checks the following? If
Then
I think that would be the correct behavior. It’s probably a little ambiguous, but it‘s good to have that tied down. |
Having slept on it, I agree completely.
TIL! I'll yank that commit. I'll see if I can sneak in the revised commits and Rubocop appeasements before my afternoon appointment. |
Co-authored-by: Denis Defreyne <[email protected]>
2ebdb48
to
a68cae2
Compare
Leading dots removed, test added for index / auto extension interaction. I had some hesitation about whether CI failed on Ruby 3.0, with what might be a test race condition about the port already being in use. ?! |
CI can be a bit flaky on occasion (never managed to reproduce the issue) but it’ll all fine after re-running! Thanks for your contribution :) |
This is now released in adsf 1.5.0 (first minor version in 6+ years!) |
Hooray! Thanks for the library. It really does a nice job of scratch a very specific itch. |
Detailed description
This adds support for implement file extensions, a la Apache MultiViews, which fixes #16. For example, a server started as follows:
…would look for first
/foo/bar.html
and then/foo/bar.htm
for the request/foo/bar
if and only if the file/foo/bar
itself does not exist.This implementation works in some quick manual tests, but before I add regression tests, docs, and CLI flags, I wanted to float the PR and make sure this is an agreeable direction for the maintainers. Let me know!
To do
(Include the to-do list for this PR to be finished here.)
Related issues