-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
[2.0 bugs] Adding nodes to the end of module bodies doesn't work with the new locals handling #259
Comments
Original comment by BitBucket: ceridwenv, GitHub: ceridwenv: I've tracked down the reason for this problem. It turns out that numpy's |
Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: PCManticore): In this case, the old way of patching locals was a lot simpler to deal with and it wasn't needing understanding of dynamically building But after investigating this a couple of days back, this wasn't totally caused by not understanding Let me use the same line of thought that I wrote on IRC: First, Did you take a look at this behaviour by any chance? |
Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: PCManticore): In fact, you might be right. The situation I described was occurring when I modified the transform to put stuff directly into locals, which manifested the behaviour with locals, so ignore my comment. In this case, a simpler solution would be to take the last |
Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: PCManticore): Only in wildcard_import_names for now. |
Original comment by BitBucket: ceridwenv, GitHub: ceridwenv: Unfortunately, I've found another test failure that's from the same underlying cause. I think we need to fix this in the module transforms, not elsewhere. FAIL: test_parser (unittest_brain.DateutilBrainTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "~/code/astroid/.tox/py34/lib/python3.4/site-packages/astroid/tests/unittest_brain.py", line 453, in test_parser
self.assertEqual(d_type.qname(), "datetime.datetime")
AssertionError: 'builtins.tuple' != 'datetime.datetime'
- builtins.tuple
+ datetime.datetime brain_dateutil adds a parse() function at the end of the dateutil module, AFAICT, and so the parse() function already defined in dateutil ends up first in the list from get_locals() and is used instead of the parse() defined in brain_dateutil. I see two reasonable ways to fix this: add the module extension nodes to the beginning of the modules rather than the end so the inference's heuristic of "always pick the first instance of a name" works, or do node replacement if a node with the same name is already available in the module to be extended. |
Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: PCManticore): Replacement sounds better, since we don't know what would happen if the same node were to be placed before the others. For instance, it might be something like this:
It also sounds that we should have an API exposed especially for this, instead of doing manual searches all the time for every transform. Something along the lines of astroid.replace_node(tree, node, 'name')? |
Original comment by BitBucket: ceridwenv, GitHub: ceridwenv: The replacement solution is very hard to get right, as I've been fighting with it on and off since yesterday, and I still haven't finished it. I pushed a bookmark containing what I have in replace_with_extension_nodes. I'm going to add another proposal that I think is a good idea independently which would simplify both this and the zipper implementation. It might be simpler and less bug-prone to simply use the hack I was using at one point to handle the special values for the builtins local variables, which was registering a special implementation of get_locals() for special node types, in this case subclasses of Module for each of the extension modules. |
Originally reported by: BitBucket: ceridwenv, GitHub: ceridwenv
I'm currently getting this test failure:
The text was updated successfully, but these errors were encountered: