How to document an "unusual" API?

[airstruck]

I've been making a few changes to a module containing a small utility function. The function is fairly simple, and the behavior is fairly straightforward. I'm having a hard time with the documentation, though. I'm never quite sure how to handle formal documentation for anything that doesn't line up neatly with procedural or OO paradigms.

In particular, the function exposed by the module takes user-defined functions as its parameters, it passes generated functions into those UDFs to be invoked by the user, and returns yet another function. There are a lot of anonymous functions to document, basically. Actual use is very simple, but the documentation makes it look complicated.

Does anyone have any tips on how to write decent documentation for this kind of thing?

[Kingdaro]

It seems like the best thing you can do is just give an example of how its used. I think the examples in the docs are good enough to give a good picture.

[ivan]

Hi there.
I strongly believe that having trouble naming functions or documenting them, is a sign that something is wrong with the code.
Looking at the code, I have a hard time figuring out what's going on, which doesn't matter right now, but it suggests that if you come back to this code yourself in a few months - you'll be scratching your head too.
Also, please note that you have to be careful when using "local function" and "return function" because these have their own scope which is a strain on the GC (edit: either that or affects the stack size - I have to look into it).
Not sure how often the code is executed, but it may affect performance and memory usage.
There are simple ways to avoid "local function" and "return function".
For example, "local links = nil" could be the first line, since "links" is used in all of your functions.
Then I would move "chain" and "Invoker" outside of the returned function too.
Good luck.

[substitute541]

Here's an inline documentation example (after spending a while trying to understand the code) :

Code: Select all

--- Chain factory
--
-- Given zero or more *link* functions, it returns a callable Chain instance.
--
-- @param function... Zero or more link functions.
--
-- @return function A Chain instance.
return function (...)
	local links = { ... }

	--- Invoker submodule
	--
	-- @access private
	--
	-- @param index The location on where start running along the chain.
	-- @return function A callable Invoker object.
	local function Invoker(index)

		--- Invoker object
		--
		-- Runs a link function.
		--
		-- Each link must have a first argument that takes a callback function
		-- `go`. Calling this function will run the next link in the chain.
		--
		-- A link may return another Chain instance instead of calling `go`. In
		-- this case, the next link (if any) will be appended to the returned
		-- Chain instance. This allows for the creation of APIs with functions
		-- that return chains rather than accepting callbacks.
		--
		-- A link should either call the callback function `go`, or return a
		-- Chain instance. It should not do both.
		--
		-- @see chain
		--
		-- @class function
		-- @name invoker
		--
		-- @param mixed... Extra link arguments. This can be passed by either
		--                 the previous link, or in the case of the first link,
		--                 by the Chain instance.
		return function (...)
			local link = links[index]

			if not link then
				return
			end

			local go = Invoker(index + 1)
			local returned = link(go, ...)

			if returned then
				returned(function (_, ...) go(...) end)
			end
		end
	end

	--- Chain object
	--
	-- Given several link functions, returns a new Chain instance with the link
	-- functions appended to the chain.
	--
	-- @class function
	-- @name chain
	--
	-- @param function... Link functions.
	-- @return function A new Chain instance, with the link functions appended
	--                  to the list.

	--- Chain object
	--
	-- Run the chain links. If passed with an initial `nil` argument, the
	-- subsequent arguments are passed into the first link.
	--
	-- @usage
	--     new_chain = Chain(
	--         function (go, foo)
	--             print foo
	--             go('Hello Lua')
	--         end,
	--         function (go, bar)
	--             print bar
	--             go()
	--         end,
	--         function (go)
	--             print 'link 3'
	--             go()
	--         end,
	--     )
	--     new_chain(nil, 'Hello World') -- >>> Hello World
	--                                   --   | Hello Lua
	--
	-- @class function
	-- @name chain
	--
	-- @param nil|mixed... Optional. An initial `nil` value, followed by the
	--                     extra arguments.

	local function chain(...)
		if not (...) then
			return Invoker(1)(select(2, ...))
		end

		local offset = #links

		for index = 1, select('#', ...) do
			links[offset + index] = select(index, ...)
		end

		return chain
	end

	return chain
end

Edit: going on what @ivan says, I also suggest refactoring the code and/or finding a different way to do this thing. It's incredibly confusing.

Edit 2: There may be some minor errors. I suggest giving it a review if you plan on using this.

[Kingdaro]

I strongly believe that having trouble naming functions or documenting them, is a sign that something is wrong with the code.

A super good point, something a lot of people tend to miss, including myself! Though I can't really think of any better way for the library to work, other than the common "instance = module.new()" and "instance:stuff()" methods, which would probably just make it look a little bloated in use.

[airstruck]

It seems like the best thing you can do is just give an example of how its used. I think the examples in the docs are good enough to give a good picture.

Examples are pretty important for something like this, I agree. I want anyone looking at it to be able to immediately figure out what it's for. It might be alright with a few more examples.

I strongly believe that having trouble naming functions or documenting them, is a sign that something is wrong with the code.

The two internal functions only have names because they refer to themselves and each other. The names aren't part of the API; the user doesn't have to deal with remembering any names, only function signatures.

The first internal function is never exposed, so there's no reason to have user-facing documentation for it. The second is returned by the "chain factory" and by "chain instances." It represents a "chain instance" itself, and it's named "chain" internally. I'm open to suggestions for better names for things, but this really doesn't seem that bad to me.

Also, please note that you have to be careful when using "local function" and "return function" because these have their own scope which is a strain on the GC [...] For example, "local links = nil" could be the first line, since "links" is used in all of your functions.

The closure is there for a reason. The internal functions operate on the "links" table defined by the "chain factory" function exposed by the module. If "links" were moved out of the closure, you'd have all chains sharing one set of links, instead of each chain having its own links. This wouldn't give the desired behavior.

The functions could be moved out of the closure and could accept "links" as a parameter instead (procedural approach), except that the user never has access to "links" and wouldn't be able to pass them in when executing the chain. Even if they could, I don't think this API would be very pleasant.

The other option would be an OO approach, essentially attaching the user-facing function to the table and returning that table instead of just returning a function in a closure (as Kingdaro hinted at). The API could actually stay the same with __call, but the "links" table is exposed which I don't really want. It's supposed to be immutable except through that user-facing function, which is why it's created from varargs in the first place instead of letting the user pass in a table that they'd have a handle to and could mess with later.

In any case, I'm not convinced that the OO approach really has any benefit over closures in this case. I'd be interested in seeing a performance test, though.

Update: I've moved one of the internal functions out of the closure in the latest commit. Might make a slight difference, but the code still relies heavily on closures. Anyway, state has to be stored somewhere; I figure closures are as good a place to store it as any.

Here's an inline documentation example

Thanks, I'll run that through Ldoc later and see what happens. In the past I've had trouble getting Ldoc to do anything useful with annotations on internal stuff. Just looking at it, I'm wondering if the only thing that will show up is the very first annotation for "chain factory".

I also suggest refactoring the code and/or finding a different way to do this thing. It's incredibly confusing.

I'm open to concrete, viable suggestions on refactoring it. The way it's done now is the simplest way I can think of to do it. If you can think of a simpler or clearer way, I'd be happy to use that instead.

[substitute541]

I took a stab at refactoring the code; indeed what you did is the best and the most concise way to do it.

Full disclosure: I have not tested this yet, I just did some guesses as to how it would run.

Code: Select all

return function ( ... )
	local chain = { ... }

	--- Invoker submodule
	--
	-- @access private
	--
	-- @param int index The location on where start running along the chain.
	-- @return function A callable Invoker object.
	local function Invoker( index )

		--- Invoker object
		--
		-- Runs a link function.
		--
		-- Each link must have a first argument that takes a callback function
		-- `go`. Calling this function will run the next link in the chain.
		--
		-- A link may return another Chain instance instead of calling `go`. In
		-- this case, the next link (if any) will be appended to the returned
		-- Chain instance. This allows for the creation of APIs with functions
		-- that return chains rather than accepting callbacks.
		--
		-- A link should either call the callback function `go`, or return a
		-- Chain instance. It should not do both.
		--
		-- @see chain
		--
		-- @class function
		-- @name invoker
		--
		-- @param mixed... Extra link arguments. This can be passed by either
		--                 the previous link, or in the case of the first link,
		--                 by the Chain instance.
		return function ( ... )
			local link = chain[ index ]

			if not link then
				return
			end

			local go = Invoker( index + 1 )
			local returned = link(go, ...)

			if returned then
				returned(function (_, ...) go(...) end)
			end
		end
	end

	--- Run the chained functions
	--
	-- @param mixed... Optional. Arguments to pass through the inital link
	--                 function.
	local function run( ... )
		local i = Invoker( 1 )
		i( ... )
	end

	--- Append functions to the chain
	--
	-- @param function... Zero or more functions to append.
	local function append( ... )
		local arg = { ... }
		local offset = #chain

		for i = 1, #arg do
			chain[ offset + i ] = arg[ i ]
		end
	end

	--- Calls either run or append depending on arguments
	--
	-- If this is called with no arguments, it runs the entire chain.
	--
	-- If this is called with one or more arguments plus initial `nil` value,
	-- (i.e. `( nil, 'foo', 'bar', 1 )`) the arguments after the initial `nil`
	-- value is passed to the initial link function of the chain.
	--
	-- If this is called with functions or callable objects as the arguments,
	-- those are appended to the chain.
	--
	-- @param mixed...
	-- @return
	local function run_or_append( ... )
		if not ( ... ) then
			run( select( 2, ... ) )
		end

		append( ... )
	end

	return {
		__call = run_or_append,
		run = run,
		append = append
	}
end

[airstruck]

I have not tested this yet, I just did some guesses as to how it would run.

You can run the tests like this:

Code: Select all

git clone https://github.com/airstruck/knife.git
cd knife
# make your changes to ./knife/chain.lua
lua knife/test.lua spec/chain.lua

If all tests pass it will run silently, otherwise it will say which test failed.

It doesn't run as-is, it needs setmetatable and __call needs an initial "self" parameter. I think you should be able to do away with the closures if the initial argument to __call is the "links" table. This kind of design could work, I just felt like closures were more straightforward.

Here's a version that does away with the outer closure. The "Invoker" function still uses closures to generate the "go" functions. I'm not sure how I feel about this design, it seems awkward to me, but maybe the code is more understandable?

Here's another version that completely does away with closures, storing state in callable tables instead. I'm pretty much convinced that closures are a cleaner and more straightforward design (the expression "going around your ass to get to your elbow" comes to mind), but I'll put together a performance test later and see what happens. Anyway, I'm not sure any of this will help much with user-facing documentation.

[airstruck]

I wrote a quick benchmark to test the current version of chain against the old version before the latest round of revisions and the closure-less version from that last hastebin link. In terms of performance when invoking chains, they score like this, with higher scores meaning they run faster:

Current: 2
Previous: 3
Closure-less: 3.5

So ivan is right, we do see some performance benefits from eliminating closures. The question is whether the performance benefits are worth the awkward, roundabout code needed to eliminate closures. I'm getting hundreds of thousands of operations per second on a very low-end machine in every case, so it feels like micro-optimization at this point; I feel like more straightforward code may be worth more here than trying to squeeze extra performance out. I'll think about it more, though.

With that out of the way, if anyone has any more suggestions about the user-facing documentation, I'd love to hear them.

[substitute541]

I prefer the current version.