GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-32911: Remove the docstring attribute of AST types by serhiy-storchaka · Pull Request #7121 · python/cpython

Conversation

@serhiy-storchaka

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka added 2 commits May 25, 2018 19:04
@serhiy-storchaka
Import initial Inada's PR 5927.
881d59e
@serhiy-storchaka
Remove the DocString AST type.
8ee9227
@serhiy-storchaka
serhiy-storchaka requested review from methane and ned-deily May 25, 2018 18:20
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 25, 2018
@bedevere-bot bedevere-bot added the awaiting merge label May 25, 2018
@ned-deily ned-deily assigned ncoghlan and benjaminp and unassigned benjaminp and ncoghlan May 25, 2018
@ned-deily
ned-deily requested review from benjaminp and ncoghlan May 25, 2018 18:37
ncoghlan
ncoghlan requested changes May 27, 2018
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting merge label May 27, 2018
@bedevere-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label May 27, 2018
@ncoghlan

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane
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 methane May 27, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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;
        }
    }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
methane
methane approved these changes May 27, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels May 27, 2018
@serhiy-storchaka

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Add a news entry and bump a magic number.
eba9f13
ned-deily
ned-deily requested changes May 28, 2018
View reviewed changes

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A couple of wording improvements

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 ned-deily May 28, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"which was added"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 ned-deily May 28, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels May 28, 2018
@serhiy-storchaka
Fix wording.
4d05ad7
@serhiy-storchaka

serhiy-storchaka commented May 29, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting changes label May 29, 2018
@bedevere-bot bedevere-bot added the awaiting change review label May 29, 2018
@bedevere-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ned-deily
ned-deily approved these changes May 29, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 29, 2018
@serhiy-storchaka
serhiy-storchaka merged commit 2641ee5 into python:3.7 May 29, 2018
@bedevere-bot bedevere-bot removed the awaiting merge label May 29, 2018
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request May 29, 2018
@serhiy-storchaka @methane
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 <[email protected]>
@serhiy-storchaka
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
@serhiy-storchaka @methane
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 <[email protected]>
@ncoghlan

ncoghlan commented May 29, 2018

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag mentioned this pull request May 29, 2018
Remove check for node.docstring berkerpeksag/astor#106
Closed
@Kodiologist Kodiologist mentioned this pull request May 31, 2018
The docstring change in Python 3.7 got reverted, so we'll have to revert our corresponding changes hylang/hy#1627
Closed
Kodiologist added a commit to Kodiologist/hy that referenced this pull request Jun 6, 2018
@Kodiologist
Update docstring handling for Python 3.7
16ec46a 
See python/cpython#7121 .
@carlwgeorge carlwgeorge mentioned this pull request Jun 15, 2018
FIX: install on python 3.7 davidhalter/jedi#971
Merged
carlwgeorge added a commit to carlwgeorge/jedi that referenced this pull request Jun 15, 2018
@carlwgeorge
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 carlwgeorge mentioned this pull request Jun 15, 2018
Parse correct AST attribute for version davidhalter/jedi#1143
Merged
davidhalter pushed a commit to davidhalter/jedi that referenced this pull request Jun 16, 2018
@carlwgeorge @davidhalter
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 isidentical mentioned this pull request Nov 11, 2020
A couple of _ast related issues python/typeshed#4760
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@serhiy-storchaka @bedevere-bot @ncoghlan @methane @ned-deily @benjaminp @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.