FeedParser.parse(_:) (and friends) take a closure callback, but work is synchronous? about feedkit

[nmdias]

FeedKit uses XMLParserunder the hood, and while XMLParser will run synchronously, a closure does provide with the flexibility to run it asynchronously, if and only if the user wants to.

Attempting to explicitly remove this flexibility from the user may be making assumptions to the way he want's to handle his execution environment.

Like you suggested, on an UI bound app for example, it makes a lot of sense to do this:

DispatchQueue.global(qos: .userInitiated).async {
    // Run parsing in a background thread
    FeedParser(URL: URL)?.parse({ (result) in
        DispatchQueue.main.async {
            // Perform updates in the main thread when finished
        }
    })
}

I'm just not sure doing this explicitly for the user is the best approach.

...I do like that parse() -> Result though. When handling lots of feeds in a non-UI environment, that would certainly improve readability and ease of use.

[NSExceptional]

a closure does provide with the flexibility to run it asynchronously, if and only if the user wants to.

No, it doesn't. No more than returning a value does. A synchronous closure callback is no different from returning a value. The following blocks of code are functionally equivalent, and the user (programmer) still has to make two Dispatch calls:

func parse(_ result: @escaping (Result) -> Void) {
    self.result = result
    self.parse()
    ...
    self.result?(someResult)
}

DispatchQueue.global(qos: .userInitiated).async {
    FeedParser(URL: URL)?.parse({ (result) in
        DispatchQueue.main.async {
            // use `result`
            ...
        }
    })
}

func parse() -> Result {
    self.parse()
    ...
    return someResult
}

DispatchQueue.global(qos: .userInitiated).async {
    var result = FeedParser(URL: URL)?.parse()
    DispatchQueue.main.async {
        // use `result`
        ...
    }
}

I recommend is this approach, which alleviates any dispatching by the user of your library:

/// Documented that the closure is executed on the main thread
func parse(_ result: @escaping (Result) -> Void) {
    self.result = result
    DispatchQueue.global().async {
        self.parse()
        ...
        DispatchQueue.main.async {
            self.result?(someResult)
        }
    }
}

// Called on any given thread
FeedParser(URL: URL)?.parse({ (result) in
    // Executes async when done, on the main thread
    
    // use `result`
    ...
})

It doesn't have to execute the callback on the main thread, but wherever it chooses to execute it should be documented so the user of your library can decide what to do. No assumptions are made, but nothing is ambiguous either.

You could also provide a synchronous method that returns a Result if you wish, but an async option is a must

[nmdias]

You have me at the closure, but I'm still not 100% convinced about taking this decision from the user. I don't see why we should give FeedKit more responsibility that it should. Yet!

Again, aren't we making assumptions here?

DispatchQueue.global().async

Say you would like to specify a different priority? Or, maybe, run this synchronously? Won't you remove me of this choice?

[NSExceptional]

You don't have to take any control away from the user, you can give them a choice:

• Provide a synchronous version that returns a value (and it would be obvious said method is synchronous because it returns a value), and…

• Provide an asynchronous version that dispatches the parsing to a background thread and executes the closure on completion. Where the closure executes is up to you as I said before, just be sure to document it!

My main issue with the current implementation is that a closure callback already implies the work is being dispatched, but that's not the case. It's deceiving, in my opinion

Dispatch priorities and other options

As for this, there are a few solutions:

• Let the user worry about it and just use .global(). This is probably okay, but there will definitely be users who want to specify a different priority, or even their own queue! Which leads us to the second option.

• Make the user provide a specific queue. This is a good idea but for most people it will just be one more thing they don't want to worry about. However, a compromise can be made.

• Allow the user to provide a queue when creating a FeedParser instance. Maybe make the init take a non-optional DispatchQueue argument with a default value of DispatchQueue.global() or something. This could also be configured as a property instead. There's a few different ways to approach this idea.

I would go with the third option. The first two options are either too limiting or too overwhelming for some users, while the third option is just right for both parties :)

I always try to provide a "sane default" when designing APIs, while allowing optional additional configuration for those who need it. This makes your APIs easy to use by novices and easy to leverage by those who really know what they're doing, often with little to no sacrifices.

[nmdias]

So, following you thought, we would have these signatures, or something like it:

init?(URL: URL, queue: DispatchQueue = DispatchQueue.global())
init(data: Data, queue: DispatchQueue = DispatchQueue.global())
init(stream: InputStream, queue: DispatchQueue = DispatchQueue.global())

func parse() -> Result
func parseAsync( _ result: @escaping (Result) -> Void)

And respective calls:

Synchronous

let result = FeedParser(URL: feedURL)?.parse()

Asynchronous

FeedParser(URL: feedURL)?.parseAsync { result in 
  // main thread
}

FeedParser(URL: feedURL, queue: .global(qos: .userInitiated))?.parseAsync { result in 
  // main thread
}

Plus the data and stream initializers.

I like where you're going with this, it does seem to provide the best of both worlds without compromises.

edit

Now that I'm looking at it, maybe move the DispatchQueue to the parseAsync method. It doesn't feel right to initialise FeedParser with a DispatchQueue and then being able to call parse directly.

[NSExceptional]

Just like that! Yeah, you could move the DispatchQueue param to the parseAsync method call :)