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 lexer parameter to Syntax #1748

Merged

Conversation

patrick91
Copy link
Contributor

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate. Will do this when the PR has been reviewed
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

I was working on some code that needed to show highlighted code and I needed to use a lexer instance as I was using some custom Pygment filters and I noticed that rich doesn't allow to pass an instance of lexer, so I decided to see if I can add support for that 😊

I'm not 100% happy with having both lexer and lexer_name as arguments, but that's the easiest way to maintain compatibilty. Another option is rename lexer_name to lexer and accept Union[str, Lexer], but that would be a breaking change (which might be ok), let me know what you think 😊

Related discussion: #985

tests/test_syntax.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of minor requests.

rich/syntax.py Outdated Show resolved Hide resolved
rich/syntax.py Outdated
@@ -374,8 +381,8 @@ def highlight(
)
_get_theme_style = self._theme.get_style_for_token
try:
lexer = get_lexer_by_name(
self.lexer_name,
lexer = self.lexer or get_lexer_by_name(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Breaking this in to an if rather than an expression would allow test coverage to span both branches, and avoid that cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda prefer the previous version 😊 ideally the if would live outside the try/except, but there's logic using the lexer inside the else statement, so I've kept it there. We still need a cast (or an asser as I've done now) in this case :)

tests/test_syntax.py Outdated Show resolved Hide resolved
@willmcgugan
Copy link
Collaborator

Just thinking this through.

I think your comment about not being 100% happy with having lexer and lexer_name was a good instinct. It would be a better interface to have a single lexer argument that may be a str or a Lexer instance.

That could be construed as a breaking change, but it was always intended to be a positional argument. So it might warrant a major version bump.

Let's go with the single lexer argument. I'll decide wether it warrants a major version bump.

@patrick91
Copy link
Contributor Author

Just thinking this through.

I think your comment about not being 100% happy with having lexer and lexer_name was a good instinct. It would be a better interface to have a single lexer argument that may be a str or a Lexer instance.

I think this looks much better! Let me know what you think 😊

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Great. Just a request for a docstring and this is good to go.

@@ -348,6 +349,20 @@ def _get_token_color(self, token_type: TokenType) -> Optional[Color]:
style = self._theme.get_style_for_token(token_type)
return style.color

@property
def lexer(self) -> Optional[Lexer]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, hope it is good!

@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #1748 (de7ed16) into master (40dbc0a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1748   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          71       71           
  Lines        6914     6951   +37     
=======================================
+ Hits         6902     6939   +37     
  Misses         12       12           
Flag Coverage Δ
unittests 99.82% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
rich/syntax.py 99.62% <100.00%> (+0.01%) ⬆️
rich/live.py 100.00% <0.00%> (ø)
rich/text.py 100.00% <0.00%> (ø)
rich/traceback.py 99.55% <0.00%> (+<0.01%) ⬆️

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 3827b4a...de7ed16. Read the comment docs.

@willmcgugan willmcgugan self-assigned this Jan 5, 2022
@willmcgugan
Copy link
Collaborator

Thanks!

@willmcgugan willmcgugan merged commit 22989e4 into Textualize:master Jan 5, 2022
@patrick91 patrick91 deleted the feature/allow-to-pass-lexer-instance branch January 5, 2022 14:41
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 11, 2022
11.0.0

Added

Added max_depth arg to pretty printing Textualize/rich#1585
Added vertical_align to Table.add_row Textualize/rich#1590

Fixed

Fixed issue with pretty repr in jupyter notebook Textualize/rich#1717
Fix Traceback theme defaults override user supplied styles Textualize/rich#1786

Changed

breaking Deprecated rich.console.RenderGroup, now named rich.console.Group
breaking Syntax.__init__ parameter lexer_name renamed to lexer
Syntax constructor accepts both str and now a pygments lexer Textualize/rich#1748
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.

3 participants