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

When returning index files, prefer .html and .htm files - fixes #48 #239

Merged
merged 1 commit into from Jan 27, 2016

Conversation

Projects
None yet
2 participants
@stig
Contributor

stig commented Jan 6, 2016

After that, fall back to the old behaviour, which might end up returning
index.php, index.JS, index.GIF or anything else.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 6, 2016

Contributor

I tried to avoid having two first calls, but that function only accepts one argument and I didn't want to create an intermediate sequence in order to do so. This should be slightly faster than the old code, particularly for big directories, as it does not need to traverse the directory entry in the common(?) case of index.html or index.htm existing.

Contributor

stig commented Jan 6, 2016

I tried to avoid having two first calls, but that function only accepts one argument and I didn't want to create an intermediate sequence in order to do so. This should be slightly faster than the old code, particularly for big directories, as it does not need to traverse the directory entry in the common(?) case of index.html or index.htm existing.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 6, 2016

Contributor

This PR (if accepted) also obsoletes PRs #114 and #208.

Contributor

stig commented Jan 6, 2016

This PR (if accepted) also obsoletes PRs #114 and #208.

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 6, 2016

Member

The function is getting a little large. Perhaps it would make more sense to split it up further. For example:

(defn- find-index-file [^File dir]
  (or (existing-file (File. dir "index.html"))
      (existing-file (File. dir "index.htm"))
      (find-file-starting-with dir "index.")))
Member

weavejester commented Jan 6, 2016

The function is getting a little large. Perhaps it would make more sense to split it up further. For example:

(defn- find-index-file [^File dir]
  (or (existing-file (File. dir "index.html"))
      (existing-file (File. dir "index.htm"))
      (find-file-starting-with dir "index.")))
@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 6, 2016

Contributor

Sure, makes sense.

Contributor

stig commented Jan 6, 2016

Sure, makes sense.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 6, 2016

Contributor

How's this? I found I couldn't use find-file because it actually uses find-index-file internally.

Contributor

stig commented Jan 6, 2016

How's this? I found I couldn't use find-file because it actually uses find-index-file internally.

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 6, 2016

Member

Just check whether the file exists. If it can't be read, it's better to let it throw an exception. So something more like:

(defn- existing-file [^File dir ^String name]
  (let [file (File. dir name)]
    (if (.exists file) file)))

Also, I don't think the docstrings are necessary. Really, the other private function shouldn't have them either.

Member

weavejester commented Jan 6, 2016

Just check whether the file exists. If it can't be read, it's better to let it throw an exception. So something more like:

(defn- existing-file [^File dir ^String name]
  (let [file (File. dir name)]
    (if (.exists file) file)))

Also, I don't think the docstrings are necessary. Really, the other private function shouldn't have them either.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 6, 2016

Contributor

Thanks for the feedback! Want me to rebase the above into a single commit?

Contributor

stig commented Jan 6, 2016

Thanks for the feedback! Want me to rebase the above into a single commit?

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 7, 2016

Member

Yes please.

Member

weavejester commented Jan 7, 2016

Yes please.

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 9, 2016

Member

Thanks for the commit. Could you change the commit message to:

Prefer HTML index files in file-response function

I think that more succinctly describes the gist of the change. I'll merge after that.

Member

weavejester commented Jan 9, 2016

Thanks for the commit. Could you change the commit message to:

Prefer HTML index files in file-response function

I think that more succinctly describes the gist of the change. I'll merge after that.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 10, 2016

Contributor

Done.

Contributor

stig commented Jan 10, 2016

Done.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 27, 2016

Contributor

Are you having second thoughts about merging this?

Contributor

stig commented Jan 27, 2016

Are you having second thoughts about merging this?

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 27, 2016

Member

Nope, the notification email just slipped me by.

Member

weavejester commented Jan 27, 2016

Nope, the notification email just slipped me by.

weavejester added a commit that referenced this pull request Jan 27, 2016

Merge pull request #239 from stig/issue-48
Prefer HTML index files in file-response function

@weavejester weavejester merged commit d4e3aae into ring-clojure:master Jan 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Feb 1, 2016

@stig stig deleted the stig:issue-48 branch Feb 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment