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

Use Liquid::Drops instead of Hashes in #to_liquid #4277

Merged
merged 15 commits into from Dec 26, 2015

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Dec 22, 2015

This requires test updates but it compiles all the (rudimentary) sites on my machine.

Fixes #3224.

@jekyll/core, please review.

Initial work on using Liquid::Drops instead of Hashes.
The properties of Liquid::Drops are only evaluated when they're asked for
and therefore save computation time. This prevents a lot of GC time cleaning
up objects that are not needed, because they're not created unless requested.
Additionally, this saves time for actual computation of those values because
they can be computed only if needed.

It's funny how much it helps when you only do what is needed. Far less overhead.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 22, 2015

Member

@envygeeks How do I run this new code through Rubocop?

Member

parkr commented Dec 22, 2015

@envygeeks How do I run this new code through Rubocop?

@parkr parkr added the Enhancement label Dec 22, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 22, 2015

Contributor

You're on OS X so you'll need to install Docker: https://www.docker.com/docker-toolbox (the preferred Method for OS X.) If that doesn't work I'll create a bot that does this all automatically and posts it to the pull request and flags it as needs-manual-review.

Contributor

envygeeks commented Dec 22, 2015

You're on OS X so you'll need to install Docker: https://www.docker.com/docker-toolbox (the preferred Method for OS X.) If that doesn't work I'll create a bot that does this all automatically and posts it to the pull request and flags it as needs-manual-review.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 22, 2015

Contributor

I know a lot of Rubyist's will disagree but I always disliked def_delegators because it's less obvious what is going on to the user at quick read and during audits, I think this should be split up to def_delegator

Contributor

envygeeks commented on lib/jekyll/drops/collection_drop.rb in 82c3ee3 Dec 22, 2015

I know a lot of Rubyist's will disagree but I always disliked def_delegators because it's less obvious what is going on to the user at quick read and during audits, I think this should be split up to def_delegator

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 22, 2015

Contributor
def_delegator :@obj, :write?, :output
Contributor

envygeeks commented on lib/jekyll/drops/collection_drop.rb in 82c3ee3 Dec 22, 2015

def_delegator :@obj, :write?, :output
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 22, 2015

Contributor

Should probably throw RuntimeError like {}.freeze will.. I only say that because I'm doing the push towards freezing everything by default, though personally I'm a fan of throwing our own base errors like CannotModifyImmutableDrop < StandardError, Jekyll.const_set(:StandardError, Class.new(StandardError) so that we can rescue all our errors and let any that aren't ours surface.

Contributor

envygeeks commented on lib/jekyll/drops/immutable_drop.rb in 82c3ee3 Dec 22, 2015

Should probably throw RuntimeError like {}.freeze will.. I only say that because I'm doing the push towards freezing everything by default, though personally I'm a fan of throwing our own base errors like CannotModifyImmutableDrop < StandardError, Jekyll.const_set(:StandardError, Class.new(StandardError) so that we can rescue all our errors and let any that aren't ours surface.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Dec 22, 2015

Member

👍 Looks good to me so far!

Member

alfredxing commented Dec 22, 2015

👍 Looks good to me so far!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 23, 2015

Member

Down to 3 failures & 6 errors on the test spec. Cucumber specs run and all pass, though there's a strange issue in the reporter on travis. Maybe a cached version or something that is causing issues.

Member

parkr commented Dec 23, 2015

Down to 3 failures & 6 errors on the test spec. Cucumber specs run and all pass, though there's a strange issue in the reporter on travis. Maybe a cached version or something that is causing issues.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 24, 2015

Member

@jekyll/core I need to finish writing tests for these drops but otherwise we should be good to go 👍

Please review this PR.

Member

parkr commented Dec 24, 2015

@jekyll/core I need to finish writing tests for these drops but otherwise we should be good to go 👍

Please review this PR.

parkr added a commit that referenced this pull request Dec 26, 2015

@parkr parkr merged commit ec25dde into master Dec 26, 2015

1 check passed

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

@parkr parkr deleted the document-drops branch Dec 26, 2015

parkr added a commit that referenced this pull request Dec 26, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

This is a smell. It obfuscates a setter.

Contributor

envygeeks commented on lib/jekyll/document.rb in fcce0d5 Dec 26, 2015

This is a smell. It obfuscates a setter.

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 26, 2015

Member

Fixed in c63b51b

Member

parkr replied Dec 26, 2015

Fixed in c63b51b

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Dec 26, 2015

Member

👍 🎉

Member

alfredxing commented Dec 26, 2015

👍 🎉

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

Smell - Breaking Default Behavior - Logic Error: Do not raise in <=> unless necessary, return nil.

"string" <=> 1 # => nil
class MyClass
  # Empty
end

MyClass.new <=> 1 # => nil
Contributor

envygeeks commented on lib/jekyll/document.rb in fcce0d5 Dec 26, 2015

Smell - Breaking Default Behavior - Logic Error: Do not raise in <=> unless necessary, return nil.

"string" <=> 1 # => nil
class MyClass
  # Empty
end

MyClass.new <=> 1 # => nil
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

Smell - Obfuscated Setter: Do not use setters in if..elsif it confuses a user who can misread it and it can lead to eventual mistakes where = is corrected to == or the verse.

Contributor

envygeeks commented on lib/jekyll/url.rb in d070a77 Dec 26, 2015

Smell - Obfuscated Setter: Do not use setters in if..elsif it confuses a user who can misread it and it can lead to eventual mistakes where = is corrected to == or the verse.

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 26, 2015

Member

I like this, actually. In Go, they work around the confusing nature of the = vs == by having first the setter, then the boolean statement that follows:

if replacement = placeholders.GetReplacement(match); replacement != nil {
    return escapePath(replacement)
} else {
    return ""
}

Could I do something like that?

Member

parkr replied Dec 26, 2015

I like this, actually. In Go, they work around the confusing nature of the = vs == by having first the setter, then the boolean statement that follows:

if replacement = placeholders.GetReplacement(match); replacement != nil {
    return escapePath(replacement)
} else {
    return ""
}

Could I do something like that?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

Smell - Encapsulation - Optional - Suggestion: Errors should be encapsulated globally on an Errors class so they are obviously spotted and easily organizable as they grow.

Contributor

envygeeks commented on lib/jekyll.rb in bff1726 Dec 26, 2015

Smell - Encapsulation - Optional - Suggestion: Errors should be encapsulated globally on an Errors class so they are obviously spotted and easily organizable as they grow.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

Smell - Old Usage of Raise - Do not raise Class.new("Msg") - Use Arguments

raise Class, "Msg"
Contributor

envygeeks commented on lib/jekyll/drops/immutable_drop.rb in bff1726 Dec 26, 2015

Smell - Old Usage of Raise - Do not raise Class.new("Msg") - Use Arguments

raise Class, "Msg"
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

Smell - Encapsulation - Optional - Suggestion: Errors should be encapsulated globally on an Errors class so they are obviously spotted and easily organizable as they grow.

Contributor

envygeeks commented on lib/jekyll/drops/immutable_drop.rb in bff1726 Dec 26, 2015

Smell - Encapsulation - Optional - Suggestion: Errors should be encapsulated globally on an Errors class so they are obviously spotted and easily organizable as they grow.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 26, 2015

Contributor

Smell - Breaking Default Behavior - Logic Error: Do not raise in <=> unless necessary, return nil.

"string" <=> 1 # => nil
class MyClass
  # Empty
end

MyClass.new <=> 1 # => nil
Contributor

envygeeks commented on lib/jekyll/document.rb in 233589e Dec 26, 2015

Smell - Breaking Default Behavior - Logic Error: Do not raise in <=> unless necessary, return nil.

"string" <=> 1 # => nil
class MyClass
  # Empty
end

MyClass.new <=> 1 # => nil

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