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
Fixing scopes region by offset #429
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,14 +97,6 @@ def __call__(self, node): | |
node.sorted_children = ast.get_children(node) | ||
|
||
def _handle(self, node, base_children, eat_parens=False, eat_spaces=False): | ||
if hasattr(node, "region"): | ||
# ???: The same node was seen twice; what should we do? | ||
warnings.warn( | ||
"Node <%s> has been already patched; please report!" | ||
% node.__class__.__name__, | ||
RuntimeWarning, | ||
) | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @climbus, is there any special reason why you removed this warning? I'm a bit hesitant of removing this warning without understanding the context of why it was added in the first place, and especially since this does not just remove the warning but by removing the return statement, it would've also allowed re-patching an already patched ast. I've merged the rest of the PR which I think is good, but restored this warning for now. If you strongly believe that this warning is no longer relevant in current rope and should be removed, please re-open a new ticket so we can discuss it properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll check if it should or shouldn't be raised. |
||
base_children = collections.deque(base_children) | ||
self.children_stack.append(base_children) | ||
children = collections.deque() | ||
|
@@ -884,6 +876,9 @@ def _With(self, node): | |
children.extend(node.body) | ||
self._handle(node, children) | ||
|
||
def _AsyncWith(self, node): | ||
return self._With(node) | ||
|
||
def _child_nodes(self, nodes, separator): | ||
children = [] | ||
for index, child in enumerate(nodes): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
except ImportError: | ||
import unittest | ||
|
||
from textwrap import dedent | ||
|
||
from rope.base import libutils | ||
from rope.base.pyobjects import get_base_type | ||
from ropetest import testutils | ||
|
@@ -328,6 +330,15 @@ def test_get_scope_for_offset_for_scope_with_indent(self): | |
inner_scope = scope.get_scopes()[0] | ||
self.assertEqual(inner_scope, scope.get_inner_scope_for_offset(10)) | ||
|
||
@testutils.only_for("3.5") | ||
def test_get_scope_for_offset_for_function_scope_and_async_with_statement(self): | ||
scope = libutils.get_string_scope( | ||
self.project, | ||
"async def func():\n async with a_func() as var:\n print(var)\n", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add |
||
) | ||
inner_scope = scope.get_scopes()[0] | ||
self.assertEqual(inner_scope, scope.get_inner_scope_for_offset(27)) | ||
|
||
def test_getting_overwritten_scopes(self): | ||
scope = libutils.get_string_scope( | ||
self.project, "def f():\n pass\ndef f():\n pass\n" | ||
|
@@ -414,3 +425,35 @@ def test_get_inner_scope_for_nested_list_comprhension(self): | |
self.assertEqual(len(scope.get_scopes()[0].get_scopes()), 1) | ||
self.assertIn("j", scope.get_scopes()[0].get_scopes()[0]) | ||
self.assertIn("i", scope.get_scopes()[0].get_scopes()[0]) | ||
|
||
def test_get_scope_region(self): | ||
scope = libutils.get_string_scope( | ||
self.project, | ||
dedent( | ||
""" | ||
def func1(ala): | ||
pass | ||
|
||
def func2(o): | ||
pass""" | ||
), | ||
) | ||
|
||
self.assertEqual(scope.get_region(), (0, 47)) | ||
self.assertEqual(scope.get_scopes()[0].get_region(), (1, 24)) | ||
self.assertEqual(scope.get_scopes()[1].get_region(), (26, 47)) | ||
|
||
def test_only_get_inner_scope_region(self): | ||
scope = libutils.get_string_scope( | ||
self.project, | ||
dedent( | ||
""" | ||
def func1(ala): | ||
pass | ||
|
||
def func2(o): | ||
pass""" | ||
), | ||
) | ||
|
||
self.assertEqual(scope.get_scopes()[1].get_region(), (26, 47)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we prefix this method with an underscore for now? I don't think that
calculate_scope_regions()
is a function that we want to expose as a public interface for this class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that. It is used outside class hierarchy: https://github.com/python-rope/rope/pull/429/files#diff-17ca262a23eefd8aedad6b0b8a032ef11a5a67123f647c3edb02ad932af9fa27R111
I'll change name.
I'm thinking if i can move regions calculations to scope creation phase. But i need to think about performance. How often scope is created without needs for regions.