Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improve documentation of wrapSite, runSnaplet and serveSnaplet #44

Merged
merged 2 commits into from Sep 20, 2012

Conversation

Projects
None yet
3 participants
Contributor

ocharles commented Sep 20, 2012

  • wrapSite now clarifies that it wraps the base snaplet, not just the current snaplet.
  • runSnaplet now correctly shows the string "devel", instead of trying to link to the devel identifier.
  • serveSnaplet clearly documents both parameters, providing a hint that defaultConfig can be used. Also removed the FIXME comment.
Improve documentation of wrapSite, runSnaplet and serveSnaplet
* wrapSite now clarifies that it wraps the base snaplet, not just the current
  snaplet.
* runSnaplet now correctly shows the string "devel", instead of trying to link
  to the `devel` identifier.
* serveSnaplet clearly documents both parameters, providing a hint that
  defaultConfig can be used. Also removed the FIXME comment.

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -367,15 +367,19 @@ addRoutes rs = do
------------------------------------------------------------------------------
--- | Wraps the snaplet's routing. When all is said and done, a snaplet's
--- routes end up getting combined into a single 'Handler' action. This
--- function allows you to modify that Handler before it actually gets served.
--- This allows you to do things that need to happen for every request handled
--- by your snaplet. Here are some examples of things you might do:
+-- | Wraps the /base/ snaplet's routing. When all is said and done, a snaplet's
@gregorycollins

gregorycollins Sep 20, 2012

Owner

While you're in here: "When all is said and done" is flowery verbiage. Could you rewrite it to be clearer?

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -367,15 +367,19 @@ addRoutes rs = do
------------------------------------------------------------------------------
--- | Wraps the snaplet's routing. When all is said and done, a snaplet's
--- routes end up getting combined into a single 'Handler' action. This
--- function allows you to modify that Handler before it actually gets served.
--- This allows you to do things that need to happen for every request handled
--- by your snaplet. Here are some examples of things you might do:
+-- | Wraps the /base/ snaplet's routing. When all is said and done, a snaplet's
+-- routes end up getting combined into a single 'Handler' action. This function
+-- allows you to modify that Handler before it actually gets served. This
@gregorycollins

gregorycollins Sep 20, 2012

Owner

"Before it actually gets served" is also misleading here (wtf did that even mean?). Better to rewrite the sentence as "The wrapSite function allows you to modify this toplevel Handler."

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -367,15 +367,19 @@ addRoutes rs = do
------------------------------------------------------------------------------
--- | Wraps the snaplet's routing. When all is said and done, a snaplet's
--- routes end up getting combined into a single 'Handler' action. This
--- function allows you to modify that Handler before it actually gets served.
--- This allows you to do things that need to happen for every request handled
--- by your snaplet. Here are some examples of things you might do:
+-- | Wraps the /base/ snaplet's routing. When all is said and done, a snaplet's
+-- routes end up getting combined into a single 'Handler' action. This function
+-- allows you to modify that Handler before it actually gets served. This
+-- allows you to do things that need to happen for every request handled by your
+-- snaplet.
+--
+-- As this wrap's the base snaplet, you are able to wrap all handlers of the
@gregorycollins

gregorycollins Sep 20, 2012

Owner

s/wrap's/wraps/

@gregorycollins

gregorycollins Sep 20, 2012

Owner

Also, "as this wraps the foo, you are able to wrap the foo" could be phrased better.

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -367,15 +367,19 @@ addRoutes rs = do
------------------------------------------------------------------------------
--- | Wraps the snaplet's routing. When all is said and done, a snaplet's
--- routes end up getting combined into a single 'Handler' action. This
--- function allows you to modify that Handler before it actually gets served.
--- This allows you to do things that need to happen for every request handled
--- by your snaplet. Here are some examples of things you might do:
+-- | Wraps the /base/ snaplet's routing. When all is said and done, a snaplet's
+-- routes end up getting combined into a single 'Handler' action. This function
+-- allows you to modify that Handler before it actually gets served. This
+-- allows you to do things that need to happen for every request handled by your
+-- snaplet.
+--
+-- As this wrap's the base snaplet, you are able to wrap all handlers of the
+-- application. Here are some examples of things you might do:
+--
+-- > wrapSite (\site -> removePathTrailingSlashes >> site)
@gregorycollins

gregorycollins Sep 20, 2012

Owner

Remove this example, it's silly

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -512,10 +516,10 @@ runInitializer' mvar env b@(Initializer i) cwd = do
------------------------------------------------------------------------------
-- | Given an envirnoment and a Snaplet initializer, produce the set of
@gregorycollins

gregorycollins Sep 20, 2012

Owner

environment is spelled wrong here

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -512,10 +516,10 @@ runInitializer' mvar env b@(Initializer i) cwd = do
------------------------------------------------------------------------------
-- | Given an envirnoment and a Snaplet initializer, produce the set of
-- messages generated during initialization, a snap handler, and a cleanup
--- action. The environment is an arbitrary string such as "devel" or
--- "production". This string is used to determine the name of the config
+-- action. The environment is an arbitrary string such as \"devel\" or
+-- \"production\". This string is used to determine the name of the config
@gregorycollins

gregorycollins Sep 20, 2012

Owner

s/config/configuration/

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -512,10 +516,10 @@ runInitializer' mvar env b@(Initializer i) cwd = do
------------------------------------------------------------------------------
-- | Given an envirnoment and a Snaplet initializer, produce the set of
-- messages generated during initialization, a snap handler, and a cleanup
--- action. The environment is an arbitrary string such as "devel" or
--- "production". This string is used to determine the name of the config
+-- action. The environment is an arbitrary string such as \"devel\" or
+-- \"production\". This string is used to determine the name of the config
-- files used each snaplet. If an environment of Nothing is used, then
@gregorycollins

gregorycollins Sep 20, 2012

Owner

"used each snaplet" is missing a preposition here :)

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -546,9 +550,14 @@ combineConfig config handler = do
------------------------------------------------------------------------------
--- | Serves a top-level snaplet as a web application. Reads command-line
--- arguments. FIXME: document this.
-serveSnaplet :: Config Snap AppConfig -> SnapletInit b b -> IO ()
+-- | Serves a top-level snaplet as a web application, reading command-line
@gregorycollins

gregorycollins Sep 20, 2012

Owner

This comment would be better as:

Initialize and run a Snaplet. The 'serveSnaplet' function parses command-line arguments, runs the given Snaplet initializer, and starts an HTTP server running the Snaplet's toplevel 'Handler'.

@gregorycollins gregorycollins commented on an outdated diff Sep 20, 2012

src/Snap/Snaplet/Internal/Initializer.hs
@@ -546,9 +550,14 @@ combineConfig config handler = do
------------------------------------------------------------------------------
--- | Serves a top-level snaplet as a web application. Reads command-line
--- arguments. FIXME: document this.
-serveSnaplet :: Config Snap AppConfig -> SnapletInit b b -> IO ()
+-- | Serves a top-level snaplet as a web application, reading command-line
+-- arguments before starting a server.
+serveSnaplet :: Config Snap AppConfig
+ -- ^ The configuration of the server - you can usually pass a
@gregorycollins

gregorycollins Sep 20, 2012

Owner

Snap code style is to put the "-- ^" on the same line as the argument , i.e.

serveSnaplet :: Config Snap AppConfig      -- ^ blah blah blah
             -> SnapletInit b b            -- ^ blah blah blah
             -> IO ()

Also, "the configuration of the server" can be "the server configuration", and "you can usually pass a default Config via" can be shortened to "this is normally" or "this is often"

P.S. thanks for working on this :)

Improve documentation of wrapSite, runSnaplet and serveSnaplet
Mostly incoporating feedback from @gregorycollins on my original pull request. I
have:

* Simplified the documentation for `wrapSite` to be more direct, and removed a
  very contrived example.

* Slightly amended the documentation for runSnaplet as it doesn't return a set
  of messages at all, but rather a single Text value.

* Rewrote the documentation for serveSnaplet as requested.
Contributor

ocharles commented Sep 20, 2012

Ok, see the details of the above commit for my latest changes. I havent't changed that indentation because at least this is consistent with the file (and @mightybyte disagreed too, so maybe this should be agreed on and changed en masse).

mightybyte added a commit that referenced this pull request Sep 20, 2012

Merge pull request #44 from ocharles/improved-documentation
Improve documentation of wrapSite, runSnaplet and serveSnaplet

@mightybyte mightybyte merged commit 12d1947 into snapframework:master Sep 20, 2012

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