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

Refactor highlighting #2154

Merged
merged 3 commits into from Apr 28, 2014

Conversation

Projects
None yet
5 participants
@jpiasetz
Contributor

jpiasetz commented Mar 18, 2014

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.

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

This comment has been minimized.

@parkr

parkr Mar 18, 2014

Member

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.

This comment has been minimized.

@jpiasetz

jpiasetz Mar 18, 2014

Contributor

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

This comment has been minimized.

@mscharley

mscharley Mar 18, 2014

Contributor

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.

This comment has been minimized.

@parkr

parkr Mar 19, 2014

Member

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.

This comment has been minimized.

@jpiasetz

jpiasetz Mar 19, 2014

Contributor

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.

This comment has been minimized.

@mattr-

mattr- Mar 19, 2014

Member

Yes, we worry about it. We have to.

This comment has been minimized.

@parkr

parkr Mar 19, 2014

Member

Limit the args to what the parser supports.

I'm all for it!

This comment has been minimized.

@jpiasetz

jpiasetz Mar 19, 2014

Contributor

@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?

This comment has been minimized.

@parkr

parkr Mar 19, 2014

Member

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

@parkr

This comment has been minimized.

Member

parkr commented Mar 18, 2014

Yeah, I like this!

@parkr

This comment has been minimized.

Member

parkr commented Mar 18, 2014

/cc @jayferd

@parkr

This comment has been minimized.

Member

parkr commented Mar 20, 2014

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

Would you mind rebasing?

@jpiasetz

This comment has been minimized.

Contributor

jpiasetz commented Mar 20, 2014

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

@mscharley

This comment has been minimized.

Contributor

mscharley commented Mar 20, 2014

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

This comment has been minimized.

Contributor

jpiasetz commented Mar 20, 2014

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

@mattr-

This comment has been minimized.

Member

mattr- commented Mar 21, 2014

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-

This comment has been minimized.

Member

mattr- commented Mar 21, 2014

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

This comment has been minimized.

Member

parkr commented Mar 21, 2014

@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

This comment has been minimized.

Contributor

jpiasetz commented Mar 22, 2014

@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.

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

This comment has been minimized.

@jpiasetz

jpiasetz Mar 22, 2014

Contributor

Hmm just noticed this is already happening below. Will remove

@mattr-

This comment has been minimized.

Member

mattr- commented Mar 22, 2014

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

This comment has been minimized.

Contributor

jpiasetz commented Mar 22, 2014

@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

This comment has been minimized.

Contributor

jpiasetz commented Apr 11, 2014

@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

This comment has been minimized.

Member

parkr commented Apr 27, 2014

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!

@jpiasetz

This comment has been minimized.

Contributor

jpiasetz commented Apr 28, 2014

@parkr done

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

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

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

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

@parkr

This comment has been minimized.

Member

parkr commented Apr 28, 2014

Thanks, @jpiasetz!!

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.