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 Tree.find_token() method #1467

Merged
merged 6 commits into from
Jan 4, 2025

Conversation

makukha
Copy link
Contributor

@makukha makukha commented Sep 11, 2024

Resolves #1466.

@makukha
Copy link
Contributor 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
Contributor 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
Contributor Author

makukha commented Sep 12, 2024

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

@erezsh ready for review.

@erezsh
Copy link
Member

erezsh commented Jan 1, 2025

Sorry it tooks me this long to reply! It slipped my memory.

I think it's better to keep it as isinstance(.., Token).

@erezsh
Copy link
Member

erezsh commented Jan 1, 2025

The logic in pretty is a little different.

P.S. if you don't have time right now, let me know, and I'll make the change for you.

@makukha
Copy link
Contributor Author

makukha commented Jan 3, 2025

Thank you so much, @erezsh! This change is pretty simple, added it to PR.

@erezsh erezsh requested a review from MegaIng January 3, 2025 22:27
@MegaIng
Copy link
Member

MegaIng commented Jan 3, 2025

Looks good, however I would also like to see a test for what happens if there is a leaf node that isn't a Token instance. (i.e. it doesn't get listed).

@makukha
Copy link
Contributor Author

makukha commented Jan 3, 2025

@MegaIng Thank you for the hint. I added a non-token item at the end of the tree. As expected, it didn't affect the result.

@erezsh erezsh merged commit 9ca0c5d into lark-parser:master Jan 4, 2025
9 checks passed
@erezsh
Copy link
Member

erezsh commented Jan 4, 2025

Thanks!

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.

Syntactic sugar: Tree.find_token()
3 participants