FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-32911: Add new AST node for docstring by methane · Pull Request #5927 · python/cpython · GitHub

/ cpython Public

bpo-32911: Add new AST node for docstring - #5927

Closed
methane wants to merge 4 commits into
python:masterfrom
methane:ast-docstring-node
ConversationCommits (4)ChecksFiles changed
Closed

bpo-32911: Add new AST node for docstring#5927
methane wants to merge 4 commits into
python:masterfrom
methane:ast-docstring-node

Conversation

methane commented Feb 27, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

methane commented Feb 27, 2018

Copy link
Copy Markdown
Member Author

Some changes are not PEP 7 compliant because I've copied from old code before GH-46.

serhiy-storchaka approved these changes Mar 2, 2018
View reviewed changes

serhiy-storchaka left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added few nitpicks, but LGTM in general!

Comment thread Python/ast.c Outdated
}
}
return NULL;
return 0;

serhiy-storchaka Feb 27, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Functions in this file usually return 1 on success and 0 on error (except functions which need to return a non-negative index on success).

Comment thread Python/compile.c
if (find_ann(stmts)) {
ADDOP(c, SETUP_ANNOTATIONS);
}
if (!asdl_seq_LEN(stmts))

serhiy-storchaka Feb 27, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe move this to the bottom of this function?

methane Mar 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be before asdl_seq_GET.
And this code is copied back from 3.6. (There are extra blank line, I'll remove it)

cpython/Python/compile.c

Lines 1457 to 1459 in baa4507

if (!asdl_seq_LEN(stmts))
return 1;
st = (stmt_ty)asdl_seq_GET(stmts, 0);

Comment thread Tools/parser/unparse.py Outdated

def _DocString(self, t):
self.fill()
self.write(repr(t.s))

serhiy-storchaka Feb 27, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not fill()?

methane closed this Mar 20, 2018
serhiy-storchaka mentioned this pull request May 25, 2018
Merged
methane deleted the ast-docstring-node branch June 11, 2019 07:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL