Callback #167

Merged
merged 4 commits into from Mar 19, 2013

Conversation

Projects
None yet
2 participants
Contributor

neocotic commented Mar 18, 2013

Added a callback argument to the document function that will be called once all asynchronous tasks have been completed.

Due to the current run-and-forget approach (which is understandable for optimizing run time), this wasn't as clean as I would have liked. This could easily be improved/simplified if all asynchronous jobs (or maybe just the directory copies) were made synchronous or the flow depended on their completed (i.e. nested callbacks - yuck!).

However, I hope it meets your standards. This should fix #166.

Owner

jashkenas commented Mar 18, 2013

You're right, that is pretty icky (and costs ~10% of the total codebase size!). What's the fundamental problem with the programmatic interface being fire-and-forget as well?

Contributor

neocotic commented Mar 18, 2013

Well, I started with the obvious approach;

if files.length then nextFile() else callback?()

However, as it's the programmatic approach the callback would be getting fired when docco hasn't actually finished yet.

Imagine a scenario where someone's code runs docco programmatically and then copies the output directory somewhere else upon completed. That developer would be relying on the callback function being called once docco's done. However, maybe not all assets had been completely copied over yet, and therefore their final documentation directory would be incomplete and possibly unusable.

The reason for the fancier error handling as well is to support general practise with callback functions, where developers generally expect errors to be handed back as the first argument to their callback function and not thrown where it can't be handled.

As I've stated, nested callbacks or synchronous copies would resolved this but perhaps a library such as async would produce cleaner code. I intentionally avoided this as it added a new dependency and I'm not sure how you'd have felt about that. I have no problems trying again with async (perhaps creating a separate PR for comparisons) if you want?

This was referenced Mar 18, 2013

Contributor

neocotic commented Mar 19, 2013

As this is the main component to enabling docco to be accessible programmatically, I'm not sure code size is a huge concern (my opinion only). Also, even though the alternative (#169) is slightly bigger than this solution it is definitely cleaner and more maintainable.

Again, the final option is to remove asynchronous aspects entirely (i.e use only synchronous functions). This may not be as optimal since processing will have to wait for the copies and each file read before proceeding. However, if you're keen on keeping code size down this might be the best approach as it shouldn't really add any lines. That said; you'd probably have to implement #155 as well since exec doesn't have a synchronous option.

Please let me know if I can do anything else to help with this as I'd really like to see this functionality soon so I can use the latest stuff 😉

Contributor

neocotic commented Mar 19, 2013

Scratch last comment, I found another way (I had a doh! moment) and thought of a much simpler way to handle the callback. It's much smaller (only 4 additional LOC - in coffeescript at least) and much more readable. Assets are copied last before calling the callback, but I don't see how this could cause any issues.

Let me know what you think.

Owner

jashkenas commented Mar 19, 2013

Ah, there you go. Much better. Small tweaks before I merge:

  • Move the error default callback out of the parameter list, and into the function body.
  • Use && instead of & for the join.
Contributor

neocotic commented Mar 19, 2013

Ah yes, I did forget the extra & and the only reason I added the default argument was to save LOC 😉

Just pushed these changes so you should be able to pick them up now.

Owner

jashkenas commented Mar 19, 2013

Sorry, but did you test this?

  • It looks like there's no spaces surrounding the &&, which would cause the lines to be munged together.
  • You don't need to use ?= as callback is defined. or= will do nicely.
Contributor

neocotic commented Mar 19, 2013

I did test this and it worked for me (tested on RHEL 6.4).

I'll make the assignment operator change just now along with the padded && if you'd prefer it that way.

@neocotic neocotic commented on an outdated diff Mar 19, 2013

docco.litcoffee
configure options
exec "mkdir -p #{config.output}", ->
- exec "cp -f #{config.css} #{config.output}"
- exec "cp -fR #{config.public} #{config.output}" if fs.existsSync config.public
+ callback ?= (error) -> throw error if error
+ complete = ->
+ exec [
+ "cp -f #{config.css} #{config.output}"
+ "cp -fR #{config.public} #{config.output}" if fs.existsSync config.public
@neocotic

neocotic Mar 19, 2013

Contributor

Might have to test what happens when if isn't passed through as the array (containing a string and undefined) will still be joined (i.e. "cp -f #{config.css} #{config.output} && "). I imagine exec will handle this well but should probably check.

Contributor

neocotic commented Mar 19, 2013

Please see the latest changes which incorporate your suggestions.

@jashkenas jashkenas added a commit that referenced this pull request Mar 19, 2013

@jashkenas jashkenas Merge pull request #167 from neocotic/async-support
Callback
0b4678f

@jashkenas jashkenas merged commit 0b4678f into jashkenas:master Mar 19, 2013

neocotic deleted the neocotic:async-support branch Mar 19, 2013

@neocotic neocotic added a commit to neocotic/docco that referenced this pull request Mar 19, 2013

@neocotic neocotic cleaned merge of pull request #167 e2afa52
Contributor

neocotic commented Mar 19, 2013

Fantastic! If/when you accept #168, would you be able to do another minor release so I can pick up this version in a dependency on another project (I'm currently adding docco 0.6 support to grunt-docco)?

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