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

Added Tree.__rich__() method to make Tree a Rich renderable #1117

Merged
merged 4 commits into from
Feb 21, 2022
Merged

Conversation

erezsh
Copy link
Member

@erezsh erezsh commented Feb 21, 2022

No description provided.

@erezsh erezsh requested a review from MegaIng February 21, 2022 18:57
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2022

Codecov Report

Merging #1117 (d036e90) into master (01e16c5) will decrease coverage by 0.11%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
- Coverage   89.44%   89.32%   -0.12%     
==========================================
  Files          52       52              
  Lines        7373     7385      +12     
==========================================
+ Hits         6595     6597       +2     
- Misses        778      788      +10     
Flag Coverage Δ
unittests 89.32% <16.66%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lark/tree.py 65.97% <16.66%> (-4.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01e16c5...d036e90. Read the comment docs.

Copy link
Member

@MegaIng MegaIng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK rich, so I can't offer suggestions/feedback for that.

Maybe an extra mode extra install in the form lark[rich] similar to how regex is handle? Although I am not sure how useful this would be.

Also, based on this code, rich might be a good basis for an extended version of this showcasing how to use the Interpreter class to do top down visiting, but I am not sure about that.

Are we testing Tree.pretty? If yes, we should try to create similar tests for Tree.rich as well IMO. Maybe just making sure that all children are visited?

@erezsh
Copy link
Member Author

erezsh commented Feb 21, 2022

I think that because it's just a utility function, and not something that will be used in production code, we don't have to be too judicious about it. Also we don't have tests for pretty() afaik.

I don't know if the extra install is worth it. Maybe if we added more rich methods to other objects, like Token and InteractiveParser.

Copy link
Member

@MegaIng MegaIng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update the docs file

@erezsh
Copy link
Member Author

erezsh commented Feb 21, 2022

Thanks!

@erezsh erezsh merged commit 9e3bfd3 into master Feb 21, 2022
@erezsh erezsh changed the title Added Tree.rich() method Added Tree.__rich__() method to make Tree a Rich renderable Feb 21, 2022
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.

None yet

3 participants