classification
Title: compileall functions do not document or test return values
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Nicholas Chammas, brett.cannon, docs@python, python-dev
Priority: normal Keywords: patch

Created on 2015-11-29 22:44 by Nicholas Chammas, last changed 2015-12-27 21:18 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
compileall-doc.patch Nicholas Chammas, 2015-12-01 17:51 review
compileall.patch Nicholas Chammas, 2015-12-09 21:17 review
compileall.patch Nicholas Chammas, 2015-12-19 22:35 review
Messages (19)
msg255600 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-11-29 22:44
I'm using the public functions of Python's built-in compileall module.

https://docs.python.org/3/library/compileall.html#public-functions

There doesn't appear to be documentation of what each of these functions returns.

I figured out, for example, that compileall.compile_file() returns 1 when the file compiles successfully, and 0 if not.

If this is "official" behavior, it would be good to see it documented so that we can rely on it.

I'd be happy to submit a patch to fix this if a committer is willing to shepherd a new contributor (me) through the process. Otherwise, this is probably a quick fix for experienced contributors.
msg255653 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-01 15:36
At this point the return values are probably as official as they are going to be since these are such old functions.

If you want to write a patch, Nicholas, please see https://docs.python.org/devguide/ for instructions on how to do that.
msg255658 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-01 16:27
Exciting! I'm on it.
msg255665 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-01 17:51
OK, here's a patch.

I reviewed the doc style guide [0] but I'm not 100% sure if I'm using the appropriate tense. There are also a couple of lines that go a bit over 80 characters, but the file already had a few of those.

Am happy to make any adjustments, if necessary.

[0] https://docs.python.org/devguide/documenting.html#style-guide
msg255666 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-01 17:55
And I just signed the contributor agreement. (Some banner showed up when I attached the patch to this issue asking me to do so.)
msg255670 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-01 18:33
Thanks, Nicholas! I'll have a look when I have a chance (hopefully no later than Friday).
msg255675 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-01 19:33
:thumbsup: Take your time.
msg255677 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-01 19:40
Oh derp. It appears this is dup of issue24386. Apologies.
msg255678 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-01 19:41
Whoops, wrong issue. Reopening.
msg255897 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-04 23:23
I just realized there are no tests for this in test_compileall either. Nicholas, do you have any interest in attempting to add tests for the return values before we document them?
msg255984 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-05 22:02
Absolutely. I'll add a "bad source file" to `setUp()` [0] and check return values as part of the existing checks in `test_compile_files()` [1].

Does that sound like a good plan to you?

Also, I noticed that `compile_path()` has no tests. Should I test it as part of `test_compile_files()` or as part of a different test function?

[0] https://hg.python.org/cpython/file/tip/Lib/test/test_compileall.py#l14
[1] https://hg.python.org/cpython/file/tip/Lib/test/test_compileall.py#l57
msg256076 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-07 21:58
Your solution to the problem SGTM. And testing compile_path() can be separate method.
msg256162 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-09 21:17
I've added the tests as we discussed. A couple of comments:

* I found it difficult to reuse the existing setUp() code so had to essentially repeat a bunch of very similar code to create "bad" files. Let me know if you think there is a better way to do this.
* I'm having trouble with the test for compile_path(). Specifically, it doesn't seem to actually use the value for skip_curdir. Do you understand why?
msg256248 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-12 01:59
Do the tests take much longer with all of the added stuff in setUp()/tearDown()? It's just that all of it has to run for all tests. You could make a mixin or put all of it in a method that you selectively call and which registers the proper cleanup method.

As for skip_curdir, if you look at https://hg.python.org/cpython/file/default/Lib/compileall.py#l188 you will notice it requires the current directory to be on sys.path and I don't see you make any such change to sys.path (and if you do you can use test_importlib.util.import_state to temporarily mutate sys.path (https://hg.python.org/cpython/file/default/Lib/test/test_importlib/util.py#l165).
msg256742 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-19 22:35
Ah, I see. The setup/teardown stuff runs for each test.

So this is what I did:
* Added a method to add a "bad" source file to the source directory. It gets cleaned up with the existing teardown method.
* Used test_importlib to temporarily mutate sys.path as you recommended.

I think this is much closer to what we want. Let me know what you think.

By the way, are there any docs on test_importlib? I couldn't find any.
msg256777 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-20 17:33
The patch LGTM, Nicholas! I'll commit it when I have a chance.

As for docs on test_importlib, nothing in the tests directory is documented to prevent people from depending on it since all the code in that directory is explicitly meant for testing Python and not general use.
msg256778 - (view) Author: Nicholas Chammas (Nicholas Chammas) * Date: 2015-12-20 17:55
Alright, sounds good to me. Thank you for guiding me through the process!
msg257100 - (view) Author: Roundup Robot (python-dev) Date: 2015-12-27 21:17
New changeset 71f071f2e074 by Brett Cannon in branch 'default':
Issue #25768: Make compileall functions return booleans and document
https://hg.python.org/cpython/rev/71f071f2e074
msg257101 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-27 21:18
Thanks for the patch, Nicholas! I have added you to Misc/ACKS.

I did tweak the patch, though, to have the functions return booleans instead of 1 or 0 and thus tweaked the docs to be less specific about the return type as well as the tests only doing assertTrue and assertFalse instead of directly comparing against 1 or 0 (which would have still worked, but I prefer the weaker definition in case the return value changes in the future).
History
Date User Action Args
2015-12-27 21:18:49brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg257101

stage: commit review -> resolved
2015-12-27 21:17:12python-devsetnosy: + python-dev
messages: + msg257100
2015-12-20 17:56:00Nicholas Chammassetmessages: + msg256778
2015-12-20 17:33:52brett.cannonsetmessages: + msg256777
stage: commit review
2015-12-19 22:35:07Nicholas Chammassetfiles: + compileall.patch

messages: + msg256742
2015-12-12 01:59:32brett.cannonsetmessages: + msg256248
2015-12-09 21:17:49Nicholas Chammassetfiles: + compileall.patch

messages: + msg256162
2015-12-07 21:58:17brett.cannonsetmessages: + msg256076
2015-12-05 22:02:14Nicholas Chammassetmessages: + msg255984
2015-12-04 23:23:21brett.cannonsettitle: compileall functions do not document return values -> compileall functions do not document or test return values
messages: + msg255897
versions: + Python 3.6, - Python 3.5
2015-12-01 19:41:05Nicholas Chammassetstatus: closed -> open

messages: + msg255678
2015-12-01 19:40:38Nicholas Chammassetstatus: open -> closed

messages: + msg255677
2015-12-01 19:33:40Nicholas Chammassetmessages: + msg255675
2015-12-01 18:33:42brett.cannonsetassignee: docs@python -> brett.cannon
messages: + msg255670
2015-12-01 17:55:53Nicholas Chammassetmessages: + msg255666
2015-12-01 17:51:52Nicholas Chammassetfiles: + compileall-doc.patch
keywords: + patch
messages: + msg255665
2015-12-01 16:27:30Nicholas Chammassetmessages: + msg255658
2015-12-01 15:36:31brett.cannonsetnosy: + brett.cannon
messages: + msg255653
2015-11-29 22:44:15Nicholas Chammascreate