Skip to content

Make Statement a proper subclass of Node - #2121

Closed
elazarg wants to merge 2 commits into
python:masterfrom
elazarg:Statement_subclass_Node
Closed

elazarg wants to merge 2 commits into
python:masterfrom
elazarg:Statement_subclass_Node

Conversation

@elazarg

@elazarg elazarg commented Sep 12, 2016

Copy link
Copy Markdown
Contributor

This is the easier half or #1783 (another step from #1785)

I've added a Declaration superclass to Var, since it's not a statement. It's not very useful yet, but at least it makes sure we don't get Var when we want a statement.

Comment thread mypy/nodes.py
pass


# For now, only Var is a declaration

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.

Curious what other uses you are planning? I am hesitant to add scaffolding for things we might need in the future. In practice the future always evolves in an unanticipated direction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure it's needed; the motivation was that Var is not a category like Statement and Expression. But now I see that Argument should be together with Var too (wherever that might be) and PEP-526 might add more.

It feels like it's appropriate here, but I don't have a real Argument to support this feeling.

@gvanrossum

Copy link
Copy Markdown
Member

General question -- should I review this one or #2122?

Another general question -- do you use any kind of refactoring tool to do this refactoring?

Yet another general question -- why are you so interested in refactoring the code? It is a lot of work for us to review with relatively little benefit, and a small risk of breaking stuff.

@elazarg

elazarg commented Sep 13, 2016

Copy link
Copy Markdown
Contributor Author

should I review this one or #2122?

Whichever you would realistically consider merging. I kept this change since it has a smaller impact and less code to review. I see you've already reviewed the bigger one, so you tell me.

do you use any kind of refactoring tool to do this refactoring?

Not so much. I use PyDev as an IDE, although not often the automatic renaming since it can break things. It has occurrences highlight, marking of unused/undeclared names (there are many unused imports in the project) and simple type-based suggestions. But mostly I use simple or regex-based replace, PyDev's ability to search for symbols in the entire project, and manual work.

I would really like to be able to do automatic refactoring in Python using an IDE. I am not familiar with an effective tool for that (which must have type information), and I hope that type annotations and type checkers will improve this situation.

Why are you so interested in refactoring the code?

In general, I simply enjoy it. I like to see the code becomes tight and exact. I think that in the process the code teaches me what actually is going on in the domain (though it might be a false impression) what are the dependencies, what is common to different parts in it, etc. - these are all personal reasons.

As to why I'm interested in refactoring this project, it's because I like this project. I tried to add support for namedtuple methods simply because I needed it (to type-check my own toy bytecode-based type-checking project...). I believe that refactoring can be useful, both for enabling change and for helping outsider (like myself) to dive into the code - it was not a trivial task for me.

So I refactor as I read the code, as a way to understand it. if I do this change, where will it fail? what are the consequences? What other parts of the code will need to be changed? How readable is the result? Many changes are not readable or fail simple reasoning, type checks, or tests; I revert most of the others too.

It is a lot of work for us to review with relatively little benefit

That's a bummer. I am not really experienced in such collaborations (or in general, to be honest), so the issues of code churn are new to me. I do try to follow your advice/request: I submit PRs only when there's an issue for it, with an agreement that it should be done some day. Isn't "today" is better than someday, since the code will only become more complex, with more people involved, etc.? Where does this reasoning fail?

Regarding this and #2122, skimming most of the changes sounds reasonable to me, since they are obvious, repetitive and type-checkable (a SeeTheReturnStatement annotation would have been helpful). The parts that need to be reviewed are only the changes that are not type-annotations-only. There is a very small number of these (roughly what you've commented on).

and a small risk of breaking stuff.

That's hardly a risk when changing type annotations. In other kinds of refactoring, if the tests didn't catch the error in the change, and reviewing it does not ring alarm bells, what makes you sure that the original code was correct?


Bottom line, just tell me where does my help fit in this project. I have no desire in becoming a burden.

@gvanrossum

gvanrossum commented Sep 13, 2016 •

Copy link
Copy Markdown
Member

Thanks for the long reply, it helps. More tomorrow. [mobile]

@gvanrossum

Copy link
Copy Markdown
Member

Let's close this one. We'll either do the bigger one (#2122) or not do this at all. This kind of PR goes stale awfully quickly.

@gvanrossum gvanrossum closed this Sep 28, 2016
@elazarg
elazarg deleted the Statement_subclass_Node branch October 2, 2016 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants