Author brian.curtin
Recipients Christophe Simonis, Iztok.Kavkler, Omega_Weapon, ajaksu2, andybuckley, brian.curtin, christian.heimes, edemaine, eric.araujo, giampaolo.rodola, iki, loewis, meatballhat, michael.foord, pitrou, r.david.murray, sandro.tosi, schmir, sfllaw, tarek, tleeuwenburg@gmail.com, tmick, vstinner, weeble, wrstlprmpft
Date 2012-06-20.05:53:17
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1340171602.89.0.506345849558.issue444582@psf.upfronthosting.co.za>
In-reply-to
Content
> I don't think file is a good name.

Changed to "cmd" for command, and that's what the Unix `which` calls it as well.

> Wait, why are we even returning more than one result?  I don't see any use cases for that in the issue (though I admit I just skimmed it).  The unix which command doesn't.

I guess I just stuck with what was always there, or maybe I introduced it back in an earlier patch - can't remember. In any case, thanks for mentioning this because the common case really is just wanting one path. `which -a` prints all instances on the path, but that'd be an additional and less-used case for sure.

> Given that the function works on Windows, its behavior (looking in the current directory and in PATHEXT) should be documented.  And if that's not what exec*p* does on windows, that should be noted, I think (or fixed?)

Documented.

> I'm also not sure what the 'path' argument is for.  Does it have a use case?  If not, drop it.

I've needed this myself when maintaining systems that need to start executables via subprocess.Popen using a Path from a configuration file or other inputs. When I'd need to setup a Path which included a lot of dependencies, like for a mini-continous integration system we had, it would be nice to know where the "foo" command was coming from in my temporary build environment.

> You are doing the PATHEXT extraction regardless of platform, which I don't think is a good idea.

Changed, it's Windows specific now. I will have access to a linux box to test this out tomorrow morning, but I think the patch should work there.

> This line appears to be useless:

Removed, that must have been from an old iteration.

> Per my suggestion of dropping the list form (at least for now), the yield should become a return, with the default return value of None indicating not found.

Done. It's all returns now including a default of None.
History
Date User Action Args
2012-06-20 05:53:23brian.curtinsetrecipients: + brian.curtin, loewis, tmick, edemaine, pitrou, vstinner, wrstlprmpft, giampaolo.rodola, christian.heimes, ajaksu2, sfllaw, schmir, tarek, eric.araujo, Christophe Simonis, andybuckley, weeble, r.david.murray, tleeuwenburg@gmail.com, michael.foord, meatballhat, sandro.tosi, iki, Iztok.Kavkler, Omega_Weapon
2012-06-20 05:53:22brian.curtinsetmessageid: <1340171602.89.0.506345849558.issue444582@psf.upfronthosting.co.za>
2012-06-20 05:53:22brian.curtinlinkissue444582 messages
2012-06-20 05:53:20brian.curtincreate