weavejester / compojure

A concise routing library for Ring/Clojure
Eclipse Public License 1.0
4.08k stars 259 forks source link

Allow dotfiles to be ignored in file serving functions #174

Closed EvanHahn closed 6 years ago

EvanHahn commented 6 years ago

This PR adds a :dotfiles? option which allows files that begin with . to be ignored by routes/files and routes/resources.

This is inspired by an option from Express.js's static file middleware.

EvanHahn commented 6 years ago

Anything I can do to get this merged? No worries if not.

weavejester commented 6 years ago

Sorry, this slid off my todo list. I think rather than having a single option, it would make more sense to have a predicate function instead.

EvanHahn commented 6 years ago

Good idea. I'll do that when I get a chance.

EvanHahn commented 6 years ago

I believe I've addressed comments—dotfiles? is now a function.

weavejester commented 6 years ago

Can you change the :dotfiles? option to :exclude instead? The reason for using a function rather than making it a boolean option is to allow for more complex exclusions. So rather than:

(resources "/" {:dot-files? false})

We can write:

(resources "/" {:exclude dot-file?})

Alternatively, I'm wondering about a syntax like:

(-> (resources "/") (exclude dot-file?))

So we'd have a generic exclude route wrapper, though that does mean that we can't have defaults, which in this case might be useful (excluding dot-files is a good default to have).

EvanHahn commented 6 years ago

Hmm, interesting. That first idea seems like it might be worth adding to this PR but the second seems pretty separate (not that it's not worth doing). Your thoughts?

weavejester commented 6 years ago

I think having an :exclude option is best, as that way it can default to dot-file?

EvanHahn commented 6 years ago

Okay. I'll give that a go (again, might be a bit before I can get to it).

EvanHahn commented 6 years ago

Do you want me to make :exclude dot-file? a default? That'd be a potentially breaking change which is why I hesitate.

weavejester commented 6 years ago

Let's not make it a default initially, as you're right that that would be a breaking change.

EvanHahn commented 6 years ago

Created #180 to replace this pull request.