Make Statement a proper subclass of Node #2121
Conversation
| pass | ||
|
|
||
|
|
||
| # For now, only Var is a declaration |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 app 8000 ropriate here, but I don't have a real Argument to support this feeling.
|
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. |
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.
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.
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.
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
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. |
|
Thanks for the long reply, it helps. More tomorrow. [mobile] |
|
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. |
This is the easier half or #1783 (another step from #1785)
I've added a
Declarationsuperclass toVar, since it's not a statement. It's not very useful yet, but at least it makes sure we don't getVarwhen we want a statement.