Refactor highlighting #2154

jpiasetz · 2014-03-18T15:07:19Z

Extract some of the common logic from the renderers. @parkr was looking at what you were doing in #2148 and thought this might be a helpful refactor. Also I think the way options were being set before was a little weird for rouge. It expects line_numbers to be a boolean instead of a string.

If interested I'll clean the test up so they'll pass.

parkr · 2014-03-18T15:13:04Z

lib/jekyll/tags/highlight.rb

+
+        @options = Hash[@args.split.map do |opt|
+          key, value = opt.split('=')
+          return key.to_sym, (value || true)


Do we have to use Symbols here or can we use Strings? GHP folks get worried about memory usage if symbols, blah blah some CVE somewhere about this.

Could use strings but I think symbol are more appropriate since there a limited number of options that it can be. The problem with symbols is they can't be garbage collected but it's more a problem for long living process with unbounded symbol creation (i.e. rails). They initially consume less memory though - see: http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-talk/164890

Note also that symbols are singletons; your link hints at that as it notes "an unbounded number of unique symbol values." ie. it's only an issue if there aren't a fixed, relatively small pool of value that key can take.

Hm... I guess this feels unbounded to me as it just takes all @args, so I could hypothetically write a highlight tag with a bajillion options you have to go through and crash your system.

Haha that would be worse under the string scenario because they have a larger initial memory footprint and it wouldn't get the chance to gc them 😉. Does jekyll as a rule worry about people giving it malicious files to parse? I think where you're going leads to another refactor though. Limit the args to what the parser supports.

Yes, we worry about it. We have to.

Limit the args to what the parser supports.

I'm all for it!

@parkr a tall order.

@parkr I'll get this in shape to be merged. Then change #2148 to apply to the render method getting nil from any of the highlighters? And then make it check against a list of supported args?

That sound perfecto. You're the best, thank you!

parkr · 2014-03-18T15:13:30Z

Yeah, I like this!

parkr · 2014-03-18T15:13:37Z

/cc @jayferd

parkr · 2014-03-20T17:26:36Z

Something seems to have gone wrong – your refactor can't be merged automatically 😦

Would you mind rebasing?

jpiasetz · 2014-03-20T22:33:38Z

Rebased. Travis was not happy about downloading gems for a while though. Has that happened before?

mscharley · 2014-03-20T22:35:36Z

Seems to happen on every PR I submit to at least one of the builds.

On 21 March 2014 09:33, John Piasetzki notifications@github.com wrote:

Rebased. Travis was not happy about downloading gems for a while though.
Has that happened before?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2154#issuecomment-38229283
.

jpiasetz · 2014-03-20T23:29:03Z

@mscharley I might have a fix for it in #2160

mattr- · 2014-03-21T14:39:50Z

I'm having a hard time seeing how this is improving the design of existing code, which is the definition of refactoring. The amount of mental hoops I jump through to understand this new code is worse than the old code (not that the old code is much better).

Would you consider breaking this out into smaller methods with better names or perhaps another object all together?

mattr- · 2014-03-21T17:06:12Z

I feel like there's a lot of cognitive overhead in this refactor. If refactoring is about improving the design of existing code, I'm not sure how this qualifies. Yes, it's shorter, but it's harder to see the intent in this code and harder to see what it's doing. From my POV, it makes a lot of sense to create at least one (if not more) smaller helper objects (even if they're just Structs) in order to make this code clearer. What do you think?

parkr · 2014-03-21T23:58:58Z

@mattr- Could you please speak to which elements of this refactor you believe to create cognitive overhead? Unless I'm overlooking things, it reads fine to me.

jpiasetz · 2014-03-22T00:02:52Z

@mattr- sorry don't follow how there is more cognitive overhead? Could you maybe drop some comments inline and point to a particular area? That said here is my rough thought process.

output = context["highlighter_prefix"] + output if context["highlighter_prefix"]
output << context["highlighter_suffix"] if context["highlighter_suffix"]

to

prefix = context["highlighter_prefix"] || ""
suffix = context["highlighter_suffix"] || ""
prefix + add_code_tag(output) + suffix

I thought this was an increase in readability. You don't need to parse each if at the end of the line to figure out what it was doing.

After that I moved it up to render because I thought in theory every render should need to add the prefix and suffix (and do it in the same way).

Removed the argument from add_code tag because it's a little weird when a within the same scope a variable is passed that can already be accessed. I moved the replacement of + to - into the add_code tag because it was the place where if it's a valid css classname actually matters. I also thought it would be true for all the renders.

For the render_codehighlighter and render_rouge I noticed their returns looked pretty similar at that point so I just highlighted that.

For the Hash thing in initialize that might be dumb... 😖 I'm not super happy with this bit either way.

jpiasetz · 2014-03-22T00:06:49Z

lib/jekyll/tags/highlight.rb

      end

-      def add_code_tags(code, lang)
+      def add_code_tag(code)
+        lang = @lang.to_s.gsub("+", "-")


Hmm just noticed this is already happening below. Will remove

mattr- · 2014-03-22T00:48:00Z

I'm so sorry. 😞 I wasn't clear with what I was talking about. 😭 😭

I was specifically referring to the Hash initialization. Everything else is 👍

jpiasetz · 2014-03-22T19:46:22Z

@mattr- no worries. In writing that I noticed that the replacement had already been moved and that I could drop the context being passed. So I guess it paid off anyways 😉.

How about something more like the original but with just the if cut down? I'm not sure how to swing it with Structs. My other idea was to walk the args similiar to what liquid is doing with parsing tokens (https://github.com/Shopify/liquid/blob/master/lib/liquid/block.rb#L19)

jpiasetz · 2014-04-11T13:27:42Z

@mattr- any thoughts on the just simplifying it a little? Also happy to switch to Structs if you explain more what you were thinking?

parkr · 2014-04-27T23:18:45Z

Would you mind rebasing this, @jpiasetz? Looks like we've updated something that conflicts with your changes here. I'd love to see this refactor merged in.

@mattr-, feel free to speak up if you have the time!

Extract some of the common logic from the renderers.

jpiasetz · 2014-04-28T03:30:29Z

@parkr done

parkr · 2014-04-28T04:02:31Z

Thanks, @jpiasetz!!

parkr reviewed Mar 18, 2014
View reviewed changes

jpiasetz reviewed Mar 22, 2014
View reviewed changes

robin850 mentioned this pull request Apr 27, 2014

Avoid duplicated output using highlight tags #2264

Merged

jpiasetz and others added 3 commits April 27, 2014 23:20

Refactor highlighting

d6bd735

Extract some of the common logic from the renderers.

Context is not used within any of the renders so no need to pass it

446ca8f

Revert back to an intialize more similar to the original

cf81331

parkr added a commit that referenced this pull request Apr 28, 2014

Merge pull request #2154 from jpiasetz/refactor-highlighter

c546269

parkr merged commit c546269 into jekyll:master Apr 28, 2014

parkr added a commit that referenced this pull request Apr 28, 2014

Update history to reflect merge of #2154 [ci skip]

b5b55c9

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor highlighting #2154

Refactor highlighting #2154

jpiasetz commented Mar 18, 2014

parkr Mar 18, 2014

jpiasetz Mar 18, 2014

mscharley Mar 18, 2014

parkr Mar 19, 2014

jpiasetz Mar 19, 2014

mattr- Mar 19, 2014

parkr Mar 19, 2014

jpiasetz Mar 19, 2014

parkr Mar 19, 2014

parkr commented Mar 18, 2014

parkr commented Mar 18, 2014

parkr commented Mar 20, 2014

jpiasetz commented Mar 20, 2014

mscharley commented Mar 20, 2014

jpiasetz commented Mar 20, 2014

mattr- commented Mar 21, 2014

mattr- commented Mar 21, 2014

parkr commented Mar 21, 2014

jpiasetz commented Mar 22, 2014

jpiasetz Mar 22, 2014

mattr- commented Mar 22, 2014

jpiasetz commented Mar 22, 2014

jpiasetz commented Apr 11, 2014

parkr commented Apr 27, 2014

jpiasetz commented Apr 28, 2014

parkr commented Apr 28, 2014

Refactor highlighting #2154

Refactor highlighting #2154

Conversation

jpiasetz commented Mar 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Mar 18, 2014

parkr commented Mar 18, 2014

parkr commented Mar 20, 2014

jpiasetz commented Mar 20, 2014

mscharley commented Mar 20, 2014

jpiasetz commented Mar 20, 2014

mattr- commented Mar 21, 2014

mattr- commented Mar 21, 2014

parkr commented Mar 21, 2014

jpiasetz commented Mar 22, 2014

Choose a reason for hiding this comment

mattr- commented Mar 22, 2014

jpiasetz commented Mar 22, 2014

jpiasetz commented Apr 11, 2014

parkr commented Apr 27, 2014

jpiasetz commented Apr 28, 2014

parkr commented Apr 28, 2014