Skip to content

Get rid of MDEF and LDEF [WIP] #2555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
gvanrossum wants to merge 7 commits into from
Closed

Get rid of MDEF and LDEF [WIP] #2555

gvanrossum wants to merge 7 commits into from
+189 −218

Conversation

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Dec 12, 2016

@JukkaL what do you think? Note that the 'kind' of a node can have a number of values, it's just that all plain variables are now GDEF. This came up in #2553 (comment).

@elazarg what do you think? GDEF isn't the greatest name, but I couldn't really come up with a better one, and DEF feels too short/ambiguous.

elazarg
elazarg reviewed Dec 12, 2016
View reviewed changes
mypy/semanal.py Outdated
module_symbol: SymbolTableNode,
import_node: ImportBase) -> bool:
if (existing_symbol.kind in (LDEF, GDEF, MDEF) and
if (existing_symbol.kind in (GDEF, GDEF, GDEF) and
Copy link
Contributor

@elazarg elazarg Dec 12, 2016

Choose a reason for hiding this comment

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

== GDEF

mypy/semanal.py Outdated
if self.is_func_scope():
kind = LDEF
kind = GDEF
self.add_symbol(defn.name, SymbolTableNode(kind, defn.info), defn)
Copy link
Contributor

@elazarg elazarg Dec 12, 2016

Choose a reason for hiding this comment

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

Inline kind

@elazarg
Copy link
Contributor

elazarg commented Dec 12, 2016

I think GDEF should not exist at all, and the type of node kind should be an Optional. This resolves the naming problem and the fact that it appears almost everywhere, adding little information to the code.

@gvanrossum
Copy link
Member Author

gvanrossum commented Dec 12, 2016

Possibly, but that might be a separate refactoring. I like to have stepping stones along the way. Also this PR is disruptive enough as it is (causing everyone else merge conflicts).

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 12, 2016

Let's wait a bit before merging this -- I'm trying to remember if there are additional use cases for which the distinction may be still useful.

@gvanrossum gvanrossum changed the title Get rid of MDEF and LDEF Get rid of MDEF and LDEF [WIP] Dec 12, 2016
@gvanrossum
Copy link
Member Author

gvanrossum commented Dec 12, 2016

Let's not do this, it'll just cause merge conflicts and @JukkaL thinks the information might be useful for some additional ideas he has.

@gvanrossum
Copy link
Member Author

gvanrossum commented Dec 12, 2016

Here's some new code that's testing for MDEF:
https://github.com/python/mypy/pull/2510/files#diff-4b20f240e711dea5a3aa8a93897909aeR629

@gvanrossum gvanrossum closed this Dec 12, 2016
@gvanrossum gvanrossum deleted the simplify-gdef-mdef-ldef branch January 10, 2017 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@elazarg elazarg elazarg left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gvanrossum @elazarg @JukkaL