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

Docs and literate comments #1

Merged
merged 8 commits into from Aug 27, 2012

Conversation

Projects
None yet
3 participants
Contributor

gordonbrander commented Aug 27, 2012

Another documentation pull request.

The literate comments are the result of going through the library to understand it, and writing documentation from what I learn.

I've also added an explanation of deferred values and an example to the Readme file.

This pull request passes (merged d41ac54 into b0c9f6b).

@Gozala Gozala commented on an outdated diff Aug 27, 2012

@@ -46,6 +46,15 @@ var await = Method(function(value, callback) {
})
exports.await = await
+// Wait for a given value to resolve, then execute a callback.
@Gozala

Gozala Aug 27, 2012

Owner

Sorry for being picky here: I distinguish promise resolution and fulfillment. Promise may be resolved to an actual value or to another promise, later one may still be pending though. For example:

var d1 = defer() 
var d2 = defer()

deliver(d1, d2)

Above d1 is resolved to d2, but it's not fulfilled yet, it will be fulfilled only after d2 is fulfilled deliver(d2, 3).
Function when let's one observe value fulfillment rather then resolution, there for I'd change phrasing
to something like: "Wait for a given value to be fulfilled / delivered, then execute an appropriate callback."

@Gozala Gozala commented on an outdated diff Aug 27, 2012

@@ -46,6 +46,15 @@ var await = Method(function(value, callback) {
})
exports.await = await
+// Wait for a given value to resolve, then execute a callback.
+//
+// If the given value is an error object, calls the `onError` callback
@Gozala

Gozala Aug 27, 2012

Owner

Maybe change to "is an error object (instance of Error)` to be more clear.

@Gozala Gozala commented on an outdated diff Aug 27, 2012

@@ -46,6 +46,15 @@ var await = Method(function(value, callback) {
})
exports.await = await
+// Wait for a given value to resolve, then execute a callback.
+//
+// If the given value is an error object, calls the `onError` callback
+// (if any). Otherwise, calls the `onFulfill` callback.
+//
+// If the value passed is not an `Eventual`, `when` will execute the
+// appropriate callback immediately.
+//
+// Returns an `Eventual` object.
@Gozala

Gozala Aug 27, 2012

Owner

Returns an Eventual value fulfilled with onFulfill(fullfilmentValue) (where fullfilmentValue is what value is fulfilled with`.

@Gozala Gozala commented on an outdated diff Aug 27, 2012

function Eventual() {
// Set initial values.
this[observers] = []
this[pending] = true
this[valueOf] = null
}
+
+// Implement `await` method for `Eventual` objects.
+//
+// Waits for the deferred value to be delivered, then invokes the
+// given function.
@Gozala

Gozala Aug 27, 2012

Owner

with value it's being fulfilled / rejected with.

Owner

Gozala commented Aug 27, 2012

This is awesome thanks for doing this!! I made few comments though, I'll address them first think tomorrow (UK time) and then merge this in, unless you'll address them before me :)

Contributor

gordonbrander commented Aug 27, 2012

All your feedback makes sense to me. Happy to update this pull request with the changes.

I also plan to add more comments in either this or a future pull request.

Update comments based on pull request feedback
Makes distinction between fulfillment and resolution.
Contributor

gordonbrander commented Aug 27, 2012

The previous commit should address all your points (feel free to change as necessary). Thanks!

This pull request fails (merged 607b2a7 into b0c9f6b).

Contributor

gordonbrander commented Aug 27, 2012

Hmm, not sure what the deal is there... the commit only contains comments. From the traceback:

I'm sorry but an error occured within Travis while running your build.

Gozala added a commit that referenced this pull request Aug 27, 2012

@Gozala Gozala merged commit a8452b5 into Gozala:master Aug 27, 2012

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment