Skip to content

bpo-16355: fix a lack in inspect.getcomments() doc#428

Merged
berkerpeksag merged 5 commits intopython:masterfrom
marco-buttu:fix-issue-16355
Mar 17, 2017
Merged

bpo-16355: fix a lack in inspect.getcomments() doc#428
berkerpeksag merged 5 commits intopython:masterfrom
marco-buttu:fix-issue-16355

Conversation

@marco-buttu
Copy link
Copy Markdown
Contributor

Fix bpo-16355.

@mention-bot
Copy link
Copy Markdown

@marco-buttu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @1st1, @asvetlov, @ncoghlan and @voidspace to be potential reviewers.

@marco-buttu marco-buttu changed the title bpo-16355: fix a lack in inspect.getcomments() bpo-16355: fix a lack in inspect.getcomments() doc Mar 3, 2017
@berkerpeksag
Copy link
Copy Markdown
Member

Note: Commit message should include "Initial patch by Vajrasky Kok.".

from test.support import run_unittest, TESTFN, DirsOnSysPath, cpython_only
from test.support import MISSING_C_DOCSTRINGS, cpython_only
from test.support import (run_unittest, TESTFN, DirsOnSysPath, cpython_only,
MISSING_C_DOCSTRINGS)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like a relevant change to bpo-16355. Please send a separate PR for this.

Anyway, I prefer hanging indentation style:

from test.support import (
    MISSING_C_DOCSTRINGS, TESTFN, DirsOnSysPath, cpython_only, run_unittest,
)

self.assertEqual(inspect.getcomments(mod), '# line 1\n')
self.assertEqual(inspect.getcomments(mod.StupidGit), '# line 20\n')
# If the object source file is not available, return None
fn = "_non_existing_filename_used_for_sourcefile_test.py"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_non_existing_filename.py is both short and descriptive. Also, this way you can avoid using a fn variable.

def test_getcomments(self):
self.assertEqual(inspect.getcomments(mod), '# line 1\n')
self.assertEqual(inspect.getcomments(mod.StupidGit), '# line 20\n')
# If the object source file is not available, return None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finish sentences with full stops.

@marco-buttu
Copy link
Copy Markdown
Contributor Author

Thanks @berkerpeksag, fixed

self.assertEqual(inspect.getcomments(mod), '# line 1\n')
self.assertEqual(inspect.getcomments(mod.StupidGit), '# line 20\n')
# If the object source file is not available, return None.
co = compile("x=1", '_non_existing_filename.py', "exec")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use single quotes like the rest of the test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @berkerpeksag, done.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Mar 16, 2017

Why don't we raise an error if we can't find the source? Why returning None?

@marco-buttu
Copy link
Copy Markdown
Contributor Author

@1st1, in bpo-16355 @ezio-melotti and @bitdancer discussed about raising an error or not, and eventually they agreed to return None.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Mar 16, 2017

I see. The idea is to preserve the backwards compatibility. Then LGTM.

Python source file (if the object is a module).
Python source file (if the object is a module). If the object's source code
is unavailable, return ``None``. This could happen if the object has been
defined in C or interactive shell.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or the interactive shell

@berkerpeksag berkerpeksag merged commit 3f2155f into python:master Mar 17, 2017
@berkerpeksag
Copy link
Copy Markdown
Member

Thanks!

berkerpeksag pushed a commit to berkerpeksag/cpython that referenced this pull request Mar 17, 2017
Initial patch by Vajrasky Kok.

(cherry picked from commit 3f2155f)
berkerpeksag pushed a commit to berkerpeksag/cpython that referenced this pull request Mar 17, 2017
Initial patch by Vajrasky Kok.

(cherry picked from commit 3f2155f)
berkerpeksag added a commit that referenced this pull request Mar 17, 2017
Initial patch by Vajrasky Kok.

(cherry picked from commit 3f2155f)
berkerpeksag added a commit that referenced this pull request Mar 17, 2017
Initial patch by Vajrasky Kok.

(cherry picked from commit 3f2155f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants