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

exit site.process sooner #6239

Merged
merged 4 commits into from Jul 27, 2017

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Jul 25, 2017

  # site.rb
    def print_stats
      if @config["profile"]
        puts @liquid_renderer.stats_table
      end
    end

Since site.print_stats further runs only if @config["profile"], I feel it'd be better if the method is called only in the special scenario.

The drawback here is the duplicate if guard unchanged to prevent breaking downstream use of site.print_stats if any

/cc @jekyll/stability

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Jul 25, 2017

Member

Ci failures unrelated.. addressed in #6240

Member

ashmaroli commented Jul 25, 2017

Ci failures unrelated.. addressed in #6240

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Jul 25, 2017

Member

@ashmaroli wanna merge from master now that #6240 is in?

Member

oe commented Jul 25, 2017

@ashmaroli wanna merge from master now that #6240 is in?

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Jul 25, 2017

Member

that being said, i'm not sure whether this is useful. doesn't it make stack traces harder to parse since it's less specific?

Member

oe commented Jul 25, 2017

that being said, i'm not sure whether this is useful. doesn't it make stack traces harder to parse since it's less specific?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Jul 25, 2017

Member

doesn't it make stack traces harder to parse

not sure if its going to be so..
@fw42 can I have your inputs on this PR? Thanks 🙂

Member

ashmaroli commented Jul 25, 2017

doesn't it make stack traces harder to parse

not sure if its going to be so..
@fw42 can I have your inputs on this PR? Thanks 🙂

@parkr

I'm on the fence about this since it seems minor, but we can see what Florian thinks...

Show outdated Hide outdated lib/jekyll/site.rb
Show outdated Hide outdated lib/jekyll/site.rb
@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jul 25, 2017

Contributor

No objections from me 👍

I don't have a strong preference for where the if should be. Either works for me and I don't think this really matters too much (I would say you can probably remove the old one without a major version bump but I'll leave that up to Parker to decide).

Contributor

fw42 commented Jul 25, 2017

No objections from me 👍

I don't have a strong preference for where the if should be. Either works for me and I don't think this really matters too much (I would say you can probably remove the old one without a major version bump but I'll leave that up to Parker to decide).

Show outdated Hide outdated lib/jekyll/site.rb
@oe

oe approved these changes Jul 26, 2017

@parkr

parkr approved these changes Jul 27, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 27, 2017

Member

Thank you!!

@jekyllbot: merge +dev

Member

parkr commented Jul 27, 2017

Thank you!!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 82c219a into jekyll:master Jul 27, 2017

2 checks passed

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

jekyllbot added a commit that referenced this pull request Jul 27, 2017

@ashmaroli ashmaroli deleted the ashmaroli:shorter-process branch Jul 27, 2017

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