samuong / alpaca

A local HTTP proxy for command-line tools. Supports PAC scripts and NTLM authentication.
Apache License 2.0
196 stars 35 forks source link

Add godocs for the cancelable package, and remove an unnecessary call to server.Close() #57

Closed samuong closed 4 years ago

samuong commented 4 years ago

This PR has two separate commits, and will be merged using a fast-forward merge.

samuong commented 4 years ago

@camh- not sure what went wrong earlier with the checks, but I think it was due to me force pushing (to change the example's package name from cancelable to cancelable_test) while the checks were running, which confused GitHub Actions.

Can you take another look?

camh- commented 4 years ago

Haven't had much of a chance to look yet, but my first thoughts were that the example was too complicated. I know you kind of want to make it look real, so you have frob() which conditionally returns an error, but I think that obscures what is going on. I think you just need to demonstrate that a defer close sometimes calls close and sometimes does not. I also think it is important to get the output stuff working, otherwise nothing is tested and it may not even work.

I'll have a quick play and see.

camh- commented 4 years ago

Not sure if you think this is any better, but it is what I had in mind:

import (
        "fmt"

        "github.com/samuong/alpaca/cancelable"
)

type printCloser struct {
        msg string
}

// Close prints the printCloser's msg on stdout if it has one and clears it
// so it doesn't print it again.
func (pc *printCloser) Close() error {
        if pc.msg != "" {
                fmt.Println(pc.msg)
                pc.msg = ""
        }
        return nil
}

func ExampleCloser() {
        c := create(false)
        c.Close() // prints nothing - already closed in create()

        c = create(true)
        c.Close() // prints msg - not closed in create()

        // Output:
        // deferred close not cancelled
        // deferred close cancelled
}

func create(doCancel bool) *printCloser {
        pc := &printCloser{"deferred close not cancelled"}

        closer := cancelable.NewCloser(pc)
        defer closer.Close()

        if !doCancel {
                return pc // deferred close will cause msg to be printed
        }

        closer.Cancel()
        pc.msg = "deferred close cancelled"
        return pc
}

I liked your runtime.Caller() thing, but I couldn't make it work.

Edit: Of course, just after I post this, I realise it doesn't really work - setting pc.msg at the end does not mean the deferred close was cancelled. It would still print the same thing if it was not cancelled. But that's the gist of that I was thinking - the use of the Closer is not obscured by other incidental things.

samuong commented 4 years ago

I like it, thanks! And it passes the test.

And yeah, I wrote all the scaffolding like the frob() stuff before I realized that it wouldn't be hidden from the example. I think it looks a lot better the way you suggested.

Edit: just saw your edit. I'll take a look...

camh- commented 4 years ago

I realised what it should do as I was drifting off to sleep. The printCloser should not have that empty string test or print only once. Just print the message. That way, we can see the first one being printed twice and the second printed only once. I would initialise the string with fmt.Sprintf("doCancel: %v", doCancel) so you would see doCancel: false printed twice, but doCancel: true printed once.

samuong commented 4 years ago

@camh- sorry for the delay, I've taken your example from Slack and left it as a single example function. Not sure I really want the two examples - which are very similar - to be split further apart on the doc page. But we can revisit this later if anyone thinks it's not clear enough.