Skip to content
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

Removing NodeNG.nearest method #691

Closed
kavins14 opened this issue Sep 2, 2019 · 3 comments · Fixed by #692
Closed

Removing NodeNG.nearest method #691

kavins14 opened this issue Sep 2, 2019 · 3 comments · Fixed by #692
Labels

Comments

@kavins14
Copy link
Contributor

kavins14 commented Sep 2, 2019

EDIT:
Based on discussions below we have decided to remove this method altogether because it's not being used in astroid or pylint and that it doesn't seem to have any use in the near future.

Steps to reproduce

Run this script:

src = """
print(before)
print(random)
print(middle)
print(after)
"""
mod = astroid.parse(src)
before, _, middle, after = mod.body
print(middle.nearest([before, after]))

Current behavior

middle.nearest([before, after]) returns before.

Expected behavior

middle.nearest([before, after]) should return after since it's closer

Issue Description

The issue is that for a NodeNG node n, n.nearest method only returns the nearest node that occurs on lines before n and disregards any nodes on the lines after.

https://github.com/PyCQA/astroid/blob/33488e28677a1e980361757cdb1db0eb13c99435/astroid/node_classes.py#L582-L607

I see two possible fixes for this:

  1. Fix the method implementation so that the node that is the closest (either before or after) is returned (where precedence is given to the node before).
  2. Update the docstring so that it is in tandem with how the method currently works.

I think that fix (1) is in line with method description so I will be making a PR for this.

Note: There is no usage of NodeNG.nearest in both pylint's and astroid's source code.

python -c "from astroid import __pkginfo__; print(__pkginfo__.version)" output

2.3.0

@kavins14 kavins14 changed the title NodeNG.nearest method discrepancy between docstring and implementation NodeNG.nearest method not always returning nearest node Sep 2, 2019
@PCManticore
Copy link
Contributor

Thanks @kavins14 Makes sense to fix this, but I wonder if it's not better to remove this method altogether. Someone might use it in a pylint / astroid plugin, but we can emit a deprecation warning for the next release, followed by completely removing it in a prior release.

@kavins14
Copy link
Contributor Author

kavins14 commented Sep 8, 2019

By defining nearest as the closest node by line number, the method is essentially comparing nodes based on how the source code is written. Other than for style-related checkers this method doesn't seem to be too useful. Maybe this is why it is not used at all in pylint and astroid.
I think it would make sense to remove this method altogether. Should I create a new issue/pr to do that?

@PCManticore
Copy link
Contributor

Sounds good, we can reuse the current issue and PR for the same effect.

@kavins14 kavins14 changed the title NodeNG.nearest method not always returning nearest node Removing NodeNG.nearest method Sep 10, 2019
PCManticore pushed a commit that referenced this issue Sep 10, 2019
The NodeNG.nearest method was not working properly as outlined in #691 
and seem to not be in use at all across astroid and pylint. 

Close #691
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants