Hi, I'm Mr. K and I got sent here from #3158. I fear I may be repeating myself, but this is a pull request.

Yes -- this is the right place -- you can ask for someone to review the PR in the upper right. But @ericmjl has already done that -- probably in response to the Issue you created.

My first impressions about this change are that it introduces an awkward API for a fairly special case. But I'm open to discussion about that. The API has two ways to specify the target. Maybe we should split this into two functions the way the shortest_path functions got split out: the existing API for single targets and a new function for multiple targets case. Before doing that we should make sure that this is not just me not seeing the API correctly. Can you describe the API in a simple way? How would you tell someone how to use this function?

I completely agree that the proposed interface is awkward. I kept the target argument for reverse compatibility.

One option is to have all_simple_paths be a wrapper for another function like all_simple_paths_targets:

def all_simple_paths(G, source, target, cutoff=None):
    return all_simple_paths_targets(G, source, [target], cutoff=cutoff)

Bump.

I'd like to handle the singleton vs container input question the way we handle it in other parts of the code (like nbunch_iter). We check if the argument is in the graph. If it is in the graph we assume it is a singleton and put it in a list. After that "check" the code can be written for containers.

if target in G:
    target = [target]

Then I'd like to keep the keyword name as "target" but make it clear in the documentation that it can be a container of nodes. The idea there is that "a target" does not have to be a single node. Just as an archery target includes many concentric rings -- "I hit the target" means one of the rings was hit. It also makes backward compatibility more straightforward. :)

Does this make any sense?

Yes! I'll have a go at refactoring.

Here's the refactor.

Notes

1. In contrast to nbunch_iter, my refactored function accepts an iterable as input. This is because I rebuild the targets into a set for fast intersections later on.
2. I continue to use targets internally. That's because at times I need to iterate over each element in targets, and I'm not sure what a sensible name for those elements would be if the container was called target (and no, I think "ring" is too esoteric).

Great! Thanks --

You could use node as the name... or t ??
But using targets internally seems to read OK too.

This code looks good. And you've got the docstrings updated which should be all we need for documentation. Can you add some basic tests to the file tests/test_simple_paths.py?
Simple short tests of key functionality is what we're aiming for. I think that is all that's left before we merge.

Thanks!

How about this? Also, the test names all have all_simple_paths in them. Usually, I would move all the all_simple_paths tests into a class named TestAllSimplePaths, but I have refrained from doing so because I'm not sure what the relevant convention in this package is.

We usually do put all the tests related to a single construct (ok... and with the same setup required) into a class named TestAllSimplePaths. But obviously it is not always consistent across the package.
So, if you want to refactor the tests that's fine, but you can leave as is too.