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

Add BaseName.definition_[start,end]_position #1584

Merged
merged 2 commits into from
May 17, 2020

Conversation

pappasam
Copy link
Contributor

Provides two public (property) methods getting the (row, column) of the start / end of the definition range. Rows start with 1, columns start with 0.

Resolves #1576

Note: I cannot run tests locally so, unfortunately, I didn't write any. See comment in issue mentioned above.

Provides two public (property) methods getting the (row, column) of the
start / end of the definition range. Rows start with 1, columns start
with 0.

:rtype: Tuple[int, int]
@pappasam
Copy link
Contributor Author

I’ll write tests soon, please don’t merge until then!

@davidhalter davidhalter marked this pull request as draft May 17, 2020 09:07
@pappasam
Copy link
Contributor Author

@davidhalter I've added the tests!

@davidhalter davidhalter marked this pull request as ready for review May 17, 2020 23:13
@davidhalter davidhalter merged commit cb1730f into davidhalter:master May 17, 2020
@davidhalter
Copy link
Owner

Thanks again!

I ended up changing a few things that I didn't notice and wanted you to know:

  • I'm not using properties anymore. For most things in APIs properties are a bad idea, because you cannot modify their behavior with params anymore. 2d17b81
  • Used parametrize for tests, I like that better for tests in Python fa6194c
  • There was an AttributeError for some cases (and the return value can be None -> so it's an Optional). 8fdf16b

@pappasam
Copy link
Contributor Author

@davidhalter ah, gotcha, glad you caught those!

That said, the change in 8fdf16b may introduce a bug. The way I read it, if self._name.tree_name is None, the line definition = self._name.tree_name.get_definition() would raise a missing attribute exception, no?

We'd probably want the check to go:

if self._name.tree_name is None:
    return None
definition = self._name.tree_name.get_definition()
if definition is None:
    return self._name.tree_name.end_pos

Also, we'd probably also want to do this exact same check for get_definition_start_position, right?

@pappasam pappasam deleted the get_definition_position branch May 18, 2020 03:02
davidhalter added a commit that referenced this pull request May 18, 2020
@davidhalter
Copy link
Owner

davidhalter commented May 18, 2020

Very good point. I think we finally arrived at a solid solution :).

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.

Feature: end_line and end_column for api.classes.BaseName
2 participants