Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compileall functions do not document or test return values #69954

Closed
nchammas mannequin opened this issue Nov 29, 2015 · 19 comments
Closed

compileall functions do not document or test return values #69954

nchammas mannequin opened this issue Nov 29, 2015 · 19 comments
Assignees
Labels
docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error

Comments

@nchammas
Copy link
Mannequin

nchammas mannequin commented Nov 29, 2015

BPO 25768
Nosy @brettcannon, @nchammas
Files
  • compileall-doc.patch
  • compileall.patch
  • compileall.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/brettcannon'
    closed_at = <Date 2015-12-27.21:18:49.653>
    created_at = <Date 2015-11-29.22:44:15.622>
    labels = ['type-bug', 'docs']
    title = 'compileall functions do not document or test return values'
    updated_at = <Date 2015-12-27.21:18:49.652>
    user = 'https://github.com/nchammas'

    bugs.python.org fields:

    activity = <Date 2015-12-27.21:18:49.652>
    actor = 'brett.cannon'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2015-12-27.21:18:49.653>
    closer = 'brett.cannon'
    components = ['Documentation']
    creation = <Date 2015-11-29.22:44:15.622>
    creator = 'nchammas'
    dependencies = []
    files = ['41201', '41277', '41364']
    hgrepos = []
    issue_num = 25768
    keywords = ['patch']
    message_count = 19.0
    messages = ['255600', '255653', '255658', '255665', '255666', '255670', '255675', '255677', '255678', '255897', '255984', '256076', '256162', '256248', '256742', '256777', '256778', '257100', '257101']
    nosy_count = 4.0
    nosy_names = ['brett.cannon', 'docs@python', 'python-dev', 'nchammas']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25768'
    versions = ['Python 3.6']

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Nov 29, 2015

    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.

    @nchammas nchammas mannequin assigned docspython Nov 29, 2015
    @nchammas nchammas mannequin added docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error labels Nov 29, 2015
    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 1, 2015

    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.

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 1, 2015

    Exciting! I'm on it.

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 1, 2015

    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

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 1, 2015

    And I just signed the contributor agreement. (Some banner showed up when I attached the patch to this issue asking me to do so.)

    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 1, 2015

    Thanks, Nicholas! I'll have a look when I have a chance (hopefully no later than Friday).

    @brettcannon brettcannon assigned brettcannon and unassigned docspython Dec 1, 2015
    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 1, 2015

    👍 Take your time.

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 1, 2015

    Oh derp. It appears this is dup of bpo-24386. Apologies.

    @nchammas nchammas mannequin closed this as completed Dec 1, 2015
    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 1, 2015

    Whoops, wrong issue. Reopening.

    @nchammas nchammas mannequin reopened this Dec 1, 2015
    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 4, 2015

    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?

    @brettcannon brettcannon changed the title compileall functions do not document return values compileall functions do not document or test return values Dec 4, 2015
    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 5, 2015

    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

    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 7, 2015

    Your solution to the problem SGTM. And testing compile_path() can be separate method.

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 9, 2015

    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?

    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 12, 2015

    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).

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 19, 2015

    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.

    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 20, 2015

    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.

    @nchammas
    Copy link
    Mannequin Author

    nchammas mannequin commented Dec 20, 2015

    Alright, sounds good to me. Thank you for guiding me through the process!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 27, 2015

    New changeset 71f071f2e074 by Brett Cannon in branch 'default':
    Issue bpo-25768: Make compileall functions return booleans and document
    https://hg.python.org/cpython/rev/71f071f2e074

    @brettcannon
    Copy link
    Member

    brettcannon commented Dec 27, 2015

    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).

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant