-
Notifications
You must be signed in to change notification settings - Fork 31
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 support for Rouge syntax highlighting #5
base: master
Are you sure you want to change the base?
Conversation
def pygmentize(file_name, code, lexer = nil) | ||
if pygmentize? && lexer.present? | ||
Pygments.highlight(code, :lexer => lexer_for_filename(file_name)) | ||
def pygmentize(file_name, code, lexer = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be renamed to highlight
Thanks for opening this PR. I like the direction you took it, but I commented with a few suggestions. |
- changed pygmentize to highlight - returs code in RougeHighlither if no lexer is found
- similar to the code in the comment - enclosed logic into same namespace (no long lines of code) - small typo in RougeHighlighter
Thanks for the feedback and sorry for the late push. |
@dewski any plans on still merging this? The patch looks interesting, specially as how Rouge evolved since 2013 👀 |
@dewski Friendly ping, any chance to get this merged at some point? :) |
@@ -120,7 +112,7 @@ def inject_rblineprof | |||
end | |||
end | |||
output << "<pre class='duration'>#{times.join("\n")}</pre>" | |||
output << "<div class='code'>#{pygmentize(file_name, code.join, 'ruby')}</div>" | |||
output << "<div class='code'>#{highlight(file_name, code.join, true)}</div>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be great to use a <pre>
instead of a <div>
here, or at least allow to customize that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted #16 for this proposal.
Hey! Sorry for the delayed response, any chance we can get the conflicts resolved? Then I'll merge. |
@@ -0,0 +1,38 @@ | |||
require 'peek/rblineprof/highlighters/pygments_highlighter.rb' | |||
require 'peek/rblineprof/highlighters/rouge_highlighter.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the .rb
suffix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work, I'd love to be able to define a custom highlighter without any monkey-patching so I posted a few suggestions.
Pygments.highlight(code, :lexer => lexer_for_filename(file_name)) | ||
def highlight(file_name, code, lexer = false) | ||
if lexer | ||
SyntaxHighlighter.highlight(code, lexer_for_filename(file_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest passing SyntaxHighlighter.highlight(file_name, code)
and let SyntaxHighlighter
handle the lexer_for_filename(file_name)
part since:
- it's not the responsibility of the controller helper
- it will allow to monkey-patch
SyntaxHighlighter
more easily
module Peek | ||
module Rblineprof | ||
class SyntaxHighlighter | ||
class_attribute :highlighter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make SyntaxHighlighter.highlighter
lazy so that we can cleanly set it to a custom highlighter in an initializer? We have this need for GitLab where we're using a custom Gitlab::Highlight
class.
class SyntaxHighlighter | ||
class_attribute :highlighter | ||
|
||
def self.highlight(code, lexer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we'd receive file_name, code
and pass them to highlighter.process
.
We could make both highlighters inherit from an abstract class in which we could put the lexer_for_filename(file_name)
method to stay DRY?
I use Rouge in my Rails app so instead of installing Python and the pygments gem I forked the lib and added support for Rouge.
It also makes the
controller_helpers.rb
file a bit cleaner.Btw. I don't see any tests/specs ?