◐ Shell
reader mode source ↗
Skip to content

bpo-35766: Change format for feature_version to (major, minor)#13992

Merged
miss-islington merged 1 commit into
python:masterfrom
gvanrossum:feat-ver
Jun 12, 2019
Merged

bpo-35766: Change format for feature_version to (major, minor)#13992
miss-islington merged 1 commit into
python:masterfrom
gvanrossum:feat-ver

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Jun 11, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

(A single int is still allowed, but undocumented.)

https://bugs.python.org/issue35766

@gvanrossum gvanrossum changed the title Change format for feature_version to (major, minor) Jun 11, 2019
@gvanrossum
Change format for feature_version to (major, minor) …
5548a92 
(A single int is still allowed, but undocumented.)
@gvanrossum gvanrossum requested a review from vstinner June 11, 2019 23:42
vstinner
vstinner reviewed Jun 12, 2019
View reviewed changes

@vstinner vstinner 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.

Hide comment

You may also update test_type_comments (like replace highest = sys.version_info[1] with highest = sys.version_info[:2]).

@gvanrossum

gvanrossum commented Jun 12, 2019 via email

Copy link
Copy Markdown
Member Author

ilevkivskyi
ilevkivskyi approved these changes Jun 12, 2019
View reviewed changes

@ilevkivskyi ilevkivskyi 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.

Hide comment

LGTM. Supporting both ways during some transitional period makes sense.

@miss-islington miss-islington merged commit 10b55c1 into python:master Jun 12, 2019
@miss-islington

miss-islington commented Jun 12, 2019

Copy link
Copy Markdown
Contributor

Thanks @gvanrossum for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2019
@gvanrossum @miss-islington
bpo-35766: Change format for feature_version to (major, minor) (pytho… …
9848bda 
…nGH-13992)

(A single int is still allowed, but undocumented.)

https://bugs.python.org/issue35766
(cherry picked from commit 10b55c1)

Co-authored-by: Guido van Rossum <guido@python.org>
@bedevere-bot

bedevere-bot commented Jun 12, 2019

Copy link
Copy Markdown

GH-13993 is a backport of this pull request to the 3.8 branch.

@ilevkivskyi

ilevkivskyi commented Jun 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Oh, did I just effectively merge this? That was a surprise! I didn't know auto-merge works this way.

@ilevkivskyi

ilevkivskyi commented Jun 12, 2019

Copy link
Copy Markdown
Member

I am also fine with the current way of things, so I would be happy to just revert this as well if it is to much work to switch to tuple.

@vstinner

vstinner commented Jun 12, 2019

Copy link
Copy Markdown
Member

Oh, I was working on an PR including my proposed changes when @ilevkivskyi merged this PR. Well, that's fine. This PR fix my request, thanks @gvanrossum!

I am also fine with the current way of things, so I would be happy to just revert this as well if it is to much work to switch to tuple.

I like the new API :-) I'm fine with keeping support for feature_version=int. At least, new code can be written using tuple, and it becomes possible to plan a deprecation plan if anyone cares of abandon the old format.

I proposed PR #13994 to make the further changes that I proposed. This change introduced an inconsistency between ast.parse() and compile() API for feature_version. So I propose to make compile() feature_version private. Anyway, the parameter only makes sense for ast.parse() anyway.

@vstinner vstinner mentioned this pull request Jun 12, 2019
Merged
vstinner pushed a commit that referenced this pull request Jun 12, 2019
@miss-islington @gvanrossum @vstinner
bpo-35766: Change format for feature_version to (major, minor) (GH-13992 …
3ba2107 
) (GH-13993)

(A single int is still allowed, but undocumented.)

https://bugs.python.org/issue35766
(cherry picked from commit 10b55c1)

Co-authored-by: Guido van Rossum <guido@python.org>
@vstinner

vstinner commented Jun 12, 2019

Copy link
Copy Markdown
Member

Guido:

I'm feeling I'm being punished for something. All this is just boring work and the reason (future compatibility) seems questionable. Can you please reconsider the demand that this be changed?

Oh sorry, it wasn't my intent.

I just questioned the API. I didn't have the opportunity to have a look at it previously, but IMHO it's still ok to change it before Python 3.8. After 3.8 final release, it will be more painful to change the API.

I was fine with doing the changes myself, but you was faster than me and directly proposed a PR ;-)

Anyway, my request is now addressed and I merged further minor changes (reviewed by @ilevkivskyi).

I like the clever trick of keeping feature_version=int as an undocumented feature, for backward compatibility. I'm fine with that, I don't want to break the backward compatibility, but just advice to start with (3, 6) for new code.

Thanks everyone, "typed AST" looks like a great feature!

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@gvanrossum @DinoV
bpo-35766: Change format for feature_version to (major, minor) (pytho… …
af89b67 
…nGH-13992)

(A single int is still allowed, but undocumented.)



https://bugs.python.org/issue35766
@gvanrossum gvanrossum deleted the feat-ver branch January 28, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

@ilevkivskyi ilevkivskyi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gvanrossum @miss-islington @bedevere-bot @ilevkivskyi @vstinner @the-knights-who-say-ni