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.

Sign up for GitHub

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

Jump to bottom

Add Tree.find_token() method #1467

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add Tree.find_token() method #1467

wants to merge 4 commits into from

Conversation

makukha
Copy link

@makukha makukha commented Sep 11, 2024

Resolves #1466.

@makukha
Copy link
Author

makukha commented Sep 11, 2024

I put this method outside of "standalone" scope as it uses scan_values() that is not included as well. Not sure if this is correct.

@makukha
Copy link
Author

makukha commented Sep 11, 2024

@erezsh ready for review.

@erezsh
Copy link
Member

erezsh commented Sep 11, 2024

Overall looks good. A few requests:

  • Add a test
  • Rename typ to token_type
  • Clarify in the docstring that it is a recursive function, i.e. it will find tokens in all the subtrees.

@makukha
Copy link
Author

makukha commented Sep 12, 2024 •
edited
Loading

I changed isinstance(v, Token) to not isinstance(v, Tree) (same as in _pretty()) to avoid importing Token.

@erezsh ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Syntactic sugar: Tree.find_token()
2 participants
@makukha @erezsh