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

bpo-32911: Remove the docstring attribute of AST types by serhiy-storchaka · Pull Request #7121 · python/cpython · GitHub

/ cpython Public

bpo-32911: Remove the docstring attribute of AST types - #7121

Merged
serhiy-storchaka merged 4 commits into
python:3.7from
serhiy-storchaka:remove-ast-docstring-attr-3.7
May 29, 2018
ConversationCommits (4)ChecksFiles changed
Merged

bpo-32911: Remove the docstring attribute of AST types#7121
serhiy-storchaka merged 4 commits into
python:3.7from
serhiy-storchaka:remove-ast-docstring-attr-3.7

Conversation

serhiy-storchaka commented May 25, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

It is based on #5927 and goes one step further to Python 3.6. It doesn't introduce a new AST type DocString.

https://bugs.python.org/issue32911

ned-deily assigned ncoghlan and benjaminp and unassigned benjaminp and ncoghlan May 25, 2018
ned-deily requested review from benjaminp and ncoghlan May 25, 2018 18:37
ncoghlan requested changes May 27, 2018
View reviewed changes

ncoghlan left a comment

Copy link
Copy Markdown
Contributor

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

Thanks for this Serhiy. In addition to the b5 NEWS entry, the only other item I see missing here is a magic number bump for the bytecode (so any debug and opt-level 1 bytecode generated with the earlier alphas and betas gets regenerated with docstrings included again).

bedevere-bot commented May 27, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ncoghlan commented May 27, 2018

Copy link
Copy Markdown
Contributor

Note that while the previous 3.7.0a1 NEWS entry is modified by the current patch, there'll need to be a dedicated NEWS entry for the reversion that will then appear under 3.7.0b5.

methane reviewed May 27, 2018
View reviewed changes
Comment thread Python/ast_opt.c
return 1;
}
int docstring = isdocstring((stmt_ty)asdl_seq_GET(stmts, 0));
CALL_SEQ(astfold_stmt, stmt_ty, stmts);

methane May 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

Loop from 1 seems simpler than JoinedStr hack.

    for (int i = docstring; i < asdl_seq_LEN(stmts); i++) {
        stmt_ty st = (stmt_ty)asdl_seq_GET(stmts, i);
        if (st != NULL && !astfold_stmt(st, ctx_, optimize_)) {
            return 0;
        }
    }

serhiy-storchaka May 28, 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

A JoinedStr hack is needed for preventing interpreting a new constant string created by optimization as a docstring:

def foo():
    'Not a' + ' docstring'
    pass

but we still want to apply optimizations to the first statement.

serhiy-storchaka commented May 28, 2018

Copy link
Copy Markdown
Member Author

Sorry, I don't know Git well still, and loss a news entry when applied Inada's patch.

I didn't think that a magic number bumping is needed, because this PR affects AST. But it also affects the first line number of nodes with docstrings. I'll bump a magic number.

ned-deily requested changes May 28, 2018
View reviewed changes

ned-deily 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

A couple of wording improvements

Comment thread Misc/NEWS.d/next/Core and Builtins/2018-02-27-20-57-00.bpo-32911.cmKfco.rst Outdated
@@ -0,0 +1,4 @@
Reverted :issue:`29463`. ``docstring`` field is removed from Module, ClassDef,
FunctionDef, and AsyncFunctionDef ast nodes which is added in 3.7a1. Docstring

ned-deily May 28, 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

"which was added"

Comment thread Misc/NEWS.d/next/Core and Builtins/2018-02-27-20-57-00.bpo-32911.cmKfco.rst Outdated
@@ -0,0 +1,4 @@
Reverted :issue:`29463`. ``docstring`` field is removed from Module, ClassDef,

ned-deily May 28, 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

We should say why we are doing this. Perhaps start the entry with:
"Due to unexpected compatibility issues discovered during downstream beta testing, reverted ... "

bedevere-bot commented May 28, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka commented May 29, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

bedevere-bot commented May 29, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

@methane, @ned-deily, @ncoghlan: please review the changes made to this pull request.

serhiy-storchaka merged commit 2641ee5 into python:3.7 May 29, 2018
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request May 29, 2018
bpo-32911: Revert bpo-29463. (pythonGH-7121)
728e537 
Remove the docstring attribute of AST types and restore docstring
expression as a first stmt in their body.

Co-authored-by: INADA Naoki <methane@users.noreply.github.com>
serhiy-storchaka deleted the remove-ast-docstring-attr-3.7 branch May 29, 2018 09:06
serhiy-storchaka added a commit that referenced this pull request May 29, 2018
bpo-32911: Revert bpo-29463. (GH-7121) (GH-7197)
73cbe7a 
Remove the docstring attribute of AST types and restore docstring
expression as a first stmt in their body.

Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

ncoghlan commented May 29, 2018

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka!

berkerpeksag mentioned this pull request May 29, 2018
Closed
Kodiologist mentioned this pull request May 31, 2018
Closed
Kodiologist added a commit to Kodiologist/hy that referenced this pull request Jun 6, 2018
Update docstring handling for Python 3.7
16ec46a 
See python/cpython#7121 .
carlwgeorge mentioned this pull request Jun 15, 2018
Merged
carlwgeorge added a commit to carlwgeorge/jedi that referenced this pull request Jun 15, 2018
Parse correct AST attribute for version
d2640d3 
Earlier development versions of Python 3.7 added the docstring field to
AST nodes.  This was later reverted in Python 3.7.0b5.

https://bugs.python.org/issue29463
python/cpython#7121
carlwgeorge mentioned this pull request Jun 15, 2018
Merged
davidhalter pushed a commit to davidhalter/jedi that referenced this pull request Jun 16, 2018
Parse correct AST attribute for version
ff4a773 
Earlier development versions of Python 3.7 added the docstring field to
AST nodes.  This was later reverted in Python 3.7.0b5.

https://bugs.python.org/issue29463
python/cpython#7121
isidentical mentioned this pull request Nov 11, 2020
Closed
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

ncoghlan ncoghlan requested changes
methane methane approved these changes
ned-deily ned-deily approved these changes
benjaminp Awaiting requested review from benjaminp

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL