Ns index summary

[ieure]

This changes the generated index to use a summary of the ns docstring rather than the whole string. This change lets you put a one- or two-line summary of the ns in the docstring, followed by extensive documentation and examples, without cluttering the ns index page. The complete docstring is still displayed on the ns-specific doc page.

Additionally, this updates some dependencies and the codox version.

[ieure]

Ping.

[weavejester]

My apologies for letting this drop off my radar.

You'll first need to remove the version bump and the dependency updates. Pull requests should be focused on a single change, and they should never increment the project version (particularly since so minor a change doesn't justify a bump from 0.6.4 to 0.7.0).

The summary function itself is lopsided, in that it has a trim with one argument, but not when there are two are more. Additionally, support for multiple regular expressions is unnecessary, as regular expressions can be concatenated using "|". I'd also be inclined drop support for "\f", as I've never seen that in a source file.

[ieure]

I will pull those commits out. Do you prefer reverting them individually, or rebasing them and force pushing to the source branch? I would like to see the dependencies updated, since Clojure 1.2.x is quite old by now and it's inconvenient to download that dependency purely for this tool. I would be glad to break that out into a separate PR, if you're amenable to updating it in the first place.

I pushed an update to summary to remove the second arity, which wasn't really intended to be publicly usable.

The page break character is \f (Control-L). It's whitespace, and is useful for breaking up sections of a source file. Emacs can navigate between page markers and narrow the visible portion of the buffer to the page point is in. I added support for it so you could choose to have a larger amount of text appear in the summary — If you put a couple sentences at the top and follow them ^L, both will show up in the summary.

Both regexps are necessary to accomplish this, because regexp syntax has no way to short circuit on a match, so #"\f|\n\n" splits on both page break and two newlines — so it's actually the same as removing ^L support from summary entirely.

[weavejester]

I will pull those commits out. Do you prefer reverting them individually, or rebasing them and force pushing to the source branch?

Rebase and force-push please.

I would like to see the dependencies updated, since Clojure 1.2.x is quite old by now and it's inconvenient to download that dependency purely for this tool.

Leiningen resolves dependencies based on how "close" they are. If you specify Clojure 1.5.1 in your project.clj file, this will override any Clojure version loaded in as a second-level dependency. So the version of Clojure Codox depends on will, in most cases, have no effect on the project it's generating documentation for.

The Clojure dependency Codox relies on represents a lowest common denominator. If the version of Clojure in Codox is updated to 1.5, then functions or language features might creep into Codox that would prevent it from working on projects that rely on older dependencies. It therefore makes sense to keep the Clojure dependency in Codox as low as possible.

The page break character is \f (Control-L). It's whitespace, and is useful for breaking up sections of a source file.

Do you have any examples where this technique is in use?

[ieure]

I force-pushed.

I don't know of major projects that use page breaks, but I found a couple examples:
https://github.com/joodie/talk-functional-clojure-sequences
https://github.com/technomancy/clojure-mode

I find them helpful, and use them extensively in my code. That said, it is somewhat edge-casey and likely has low discoverability, so I would be okay removing it if you don't see value in the feature.

[weavejester]

Including \f doesn't seem like it would break anything, so let's keep it in. I just wanted to see if more than one person was using them.

[weavejester]

Your implementation of summary is still a little odd, though. This does the same thing without need for a loop/recur:

(defn summary [s]
  (str/trim (first (str/split s #"\f|\n\n"))))

Also, it would make sense to put the str/trim inside, so that it catches the rare case where a docstring starts with two leading newlines:

(defn summary [s]
  (first (str/split (str/trim s) #"\f|\n\n")))

Or:

(defn summary [s]
  (-> s str/trim (str/split #"\f|\n\n") first))

[ieure]

The use of loop/recur may seem odd, but it is deliberate. Using a simple regex is not equivalent:

codox.utils> (def docstring "This is the thinger.\n\nIt does useful things.\n\n\fYou can use it like so: (wall of text)")
#'codox.utils/docstring
codox.utils> (summary docstring)
"This is the thinger.\n\nIt does useful things."
codox.utils> (first (str/split docstring #"\f|\n\n"))
"This is the thinger."
codox.utils> 

This defeats the purpose of supporting ^L in the first place; you may as well unconditionally split on #"\n\n", because it's functionally equivalent for this case.

If you're against using loop/recur, I can turn it into a nested conditional, but I chose loop/recur to eliminate the redundancy that approach would entail.

[weavejester]

Hm, okay, so what about:

(defn summary [s]
  (re-find #"(?s).*?(?=\f)|.*?(?=\n\n)|.*" s))

[ieure]

Yeah, that works and seems cleaner — though I think the regex is harder to read. I pushed an update.

[weavejester]

I guess it depends on how much you read regular expressions :)

The str/trim should be on the inside, not the outside, but aside from that, it looks okay. Squash your commits and I'll merge.

[ieure]

If the trim happens before the re-find, trailing whitespace from the split is left:

codox.utils> (re-find #"(?s).*?(?=\f)|.*?(?=\n\n)|.*" (str/trim "Hey\n\nHi\n\f\nwhat is going on here"))
"Hey\n\nHi\n"

[weavejester]

But since this is being inserted into HTML, whether you trim the output or not doesn't affect what the user sees. On the other hand, if you have a docstring with leading newlines (for whatever reason) trimming it before passing it to the regex will produce the correct summary instead of string of whitespace.

[muhuk]

All the issues in this PR seems to be resolved. Is there any reason why it shouldn't be merged?

[weavejester]

The last issue I commented on has not been resolved.

[muhuk]

My mistake, I missed it.

[muhuk]

I've created a new pull request where trimming is done before passing the docstring to the regex.

[weavejester]

Fixed by #41