classification
Title: Incorrect variable name in importlib._bootstrap._get_sourcefile
Type: behavior Stage: resolved
Components: IO Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, madison.may, python-dev
Priority: normal Keywords: easy, patch

Created on 2013-07-03 13:11 by brett.cannon, last changed 2013-07-06 22:15 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
Issue18351.patch madison.may, 2013-07-04 17:19 review
Issue18351_Python3-3.patch madison.may, 2013-07-05 21:52 Additional bug fixes + a few test cases in test_import.py review
Messages (12)
msg192237 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-07-03 13:11
http://hg.python.org/cpython/file/8e838598eed1/Lib/importlib/_bootstrap.py#l455 : source_stats does not exist and instead should be source_path.
msg192300 - (view) Author: Madison May (madison.may) * Date: 2013-07-04 17:19
Here's a 5 character patch for the sake of completeness.
msg192304 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-07-04 19:02
Any chance you want to write some tests for the function? =)
msg192337 - (view) Author: Madison May (madison.may) * Date: 2013-07-05 17:12
I'd be glad to -- I'll get right to work =).  On a related note, rpartition is also misspelled in _get_sourcefile() on line 446.
msg192338 - (view) Author: Madison May (madison.may) * Date: 2013-07-05 17:26
You might have to bear with me -- I'm a bit new to this.  What's the protocol for functions like _get_sourcepath() that require support files for testing?  I'll should probably have a couple .pyc's, .pyo's and .py files to use for testing purposes.

Additionally, would you recommend writing tests for _get_sourcepath() in test_imp.py, or would another location prove better in your opinion?
msg192339 - (view) Author: Madison May (madison.may) * Date: 2013-07-05 17:36
Yet another _get_sourcefile() related bug to report.  Line 453 should be:

source_path = bytcode_path[:-1]

instead of 

source_path = bytcode_path[-1:]
msg192340 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-07-05 17:48
Wow, I really botched that function. And no one seems to really use it to notice the errors (or at least the branches in the function where the errors exist are never reached). =)

So the tests should go in test_importlib or test_import (I'm leaning towards the latter since this function is for the C API). As for the files, I wouldn't create any and instead mock out importlib._bootstrap._path_isfile() to return the value you want.

And one last thing, Madison, is to do the patch against Python 3.3 if you can. This obviously needs to be backported so it's best to start there and then work forward.
msg192363 - (view) Author: Madison May (madison.may) * Date: 2013-07-05 21:52
Here's a preliminary attempt at a patch and a small set of test cases for _get_sourcefile.  I fixed one more spelling error in _get_sourcefile (bytcode -> bytecode) and tweaked a bit of logic.

Instead of:

extension.lower()[-3:-1] != '.py' 

I think we needed

extension.lower()[-3:-1] != 'py'

You must have been having a rough day, Brett =).  Anyhow, when you get a chance, take a look at this first attempt and let me know what you'd do differently.

Thanks,

Madison
msg192472 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-07-06 17:29
When I wrote that code I was struggling to untangle the C code and it was driving me up the wall because it was code unique to the C API and was just ever so slightly different.

Anyway, the patch looks great! When I have time I will commit it to 3.3 and 3.4.
msg192482 - (view) Author: Madison May (madison.may) * Date: 2013-07-06 19:31
I can imagine that would be incredibly frustrating -- it would drive me up the wall as well.

Anyhow, glad to hear things looked good.  I really appreciate your help guiding me through this one.
msg192490 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-06 22:05
New changeset b8028f74bac4 by Brett Cannon in branch '3.3':
Issue #18351: Fix various issues with
http://hg.python.org/cpython/rev/b8028f74bac4

New changeset e80634ad5a0e by Brett Cannon in branch 'default':
merge for issue #18351.
http://hg.python.org/cpython/rev/e80634ad5a0e
msg192493 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-07-06 22:15
Thanks for the patch, Madison! Added you to the ACKS file.
History
Date User Action Args
2013-07-06 22:15:42brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg192493

stage: commit review -> resolved
2013-07-06 22:05:12python-devsetnosy: + python-dev
messages: + msg192490
2013-07-06 19:31:01madison.maysetmessages: + msg192482
2013-07-06 17:30:29brett.cannonsetstage: test needed -> commit review
2013-07-06 17:29:20brett.cannonsetmessages: + msg192472
2013-07-05 21:52:46madison.maysetfiles: + Issue18351_Python3-3.patch

messages: + msg192363
2013-07-05 17:48:46brett.cannonsetmessages: + msg192340
2013-07-05 17:36:49madison.maysetmessages: + msg192339
2013-07-05 17:26:12madison.maysetmessages: + msg192338
2013-07-05 17:12:02madison.maysetmessages: + msg192337
2013-07-04 19:02:42brett.cannonsetkeywords: + easy

messages: + msg192304
2013-07-04 17:19:43madison.maysetfiles: + Issue18351.patch

nosy: + madison.may
messages: + msg192300

keywords: + patch
2013-07-03 13:11:12brett.cannoncreate