<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Nov 2, 2013 at 5:46 AM, Nick Coghlan <span dir="ltr"><<a href="mailto:ncoghlan@gmail.com" target="_blank">ncoghlan@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 2 November 2013 08:44, Eric Snow <<a href="mailto:ericsnowcurrently@gmail.com">ericsnowcurrently@gmail.com</a>> wrote:<br>


> On Fri, Nov 1, 2013 at 11:59 AM, Brett Cannon <<a href="mailto:brett@python.org">brett@python.org</a>> wrote:<br>
>> Thanks for the clarification. It all SGTM.<br>
><br>
> I've updated the PEP and the reference implementation.<br>
<br>
</div>While I was saying loader when I should have been saying finder, Eric<br>
correctly divined my intent :)<br>
<div class="im"><br>
> If I've missed<br>
> anything please let me know.  There is only one thing I thought of<br>
> that I'd like to get an opinion on:<br>
><br>
> In the Finders section, the PEP specifies returning None (or using a<br>
> different loader) when the found loader does not support loading into<br>
> an existing module (e.g during reload). An alternative to returning<br>
> None would be for the finder to raise ImportError with a message like<br>
> "the loader does not support reloading the module". This may actually<br>
> be a better approach since "could not find a loader" and "the found<br>
> loader won't work" are different situations that a single return value<br>
> (None) may not sufficiently represent.<br>
<br>
</div>Throwing ImportError for "can load, but cannot load into that target<br>
module" sounds good to me.</blockquote><div><br></div><div>SGTM as well.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> We should also be explicit that throwing an<br>


exception from find_spec *stops* the search (unlike returning None).<br></blockquote><div><br></div><div>I thought that was obvious but more docs doesn't hurt. =)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div class="im"><br>
> Other than that, I'm not aware of any blockers for the PEP.<br>
<br>
</div>The one slight quibble I have with the PEP update is the wording<br>
regarding the "existing" parameter. I think runpy should pass the new<br>
parameter as well when calling find_spec, but in that case it *won't*<br>
be an existing copy of the named module, it will be<br>
sys.modules["__main__"]. So we shouldn't set an expectation that the<br>
existing module passed in will necessarily be a previous copy of the<br>
module being loaded, it's just an indication to the finder that the<br>
target namespace for execution will be a pre-existing module rather<br>
than a new one.<br>
<br>
For the same reason, I also have a mild preference for "target" (or<br>
the more explicit "load_target") as the name, although I won't object<br>
if you and Brett prefer "existing".</blockquote><div><br></div><div>I would go with "module" or "target" and that is where I shall end the bikeshedding. </div></div></div></div>