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

Colorize interpolated output in logger.info #5239

Merged
merged 1 commit into from Aug 16, 2016

Conversation

Projects
None yet
4 participants
@ashmaroli
Member

ashmaroli commented Aug 12, 2016

Colorize interpolated output in Jekyll.logger.info as a way of highlighting relevant text.

blog_path

theme_name n path


Documentation:

This does not color every interpolated string automatically.
To colorize, simply add the colorator method (use cyan for homogeneity) within the expression.

"#{variable}" -> "#{variable.cyan}"

# if an expression doesn't result in a string, convert to one and then colorize.
"#{variable.method}" -> "#{variable.method.to_s.cyan}"
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 14, 2016

Contributor

Please interpolate instead of doing "+", that's why we enforce double quotes.

Contributor

envygeeks commented Aug 14, 2016

Please interpolate instead of doing "+", that's why we enforce double quotes.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 16, 2016

Contributor

LGTM. I like it. This does improve readability.

Contributor

envygeeks commented Aug 16, 2016

LGTM. I like it. This does improve readability.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 16, 2016

Contributor

/cc @jekyll/core

Contributor

envygeeks commented Aug 16, 2016

/cc @jekyll/core

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 16, 2016

Member

LGTM.

Member

mattr- commented Aug 16, 2016

LGTM.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 16, 2016

Member

squashing commits..

Member

ashmaroli commented Aug 16, 2016

squashing commits..

Show outdated Hide outdated lib/jekyll/commands/new_theme.rb
update_logger
end
def update_logger

This comment has been minimized.

@mattr-

mattr- Aug 16, 2016

Member

I didn't notice this before, but the addition of the update_logger method complicates this command quite a bit. I think we're better off without the extra method. Please make that change, and I'll reapprove.

@mattr-

mattr- Aug 16, 2016

Member

I didn't notice this before, but the addition of the update_logger method complicates this command quite a bit. I think we're better off without the extra method. Please make that change, and I'll reapprove.

This comment has been minimized.

@ashmaroli

ashmaroli Aug 16, 2016

Member

I agree, it was added to satisfy rubocop with an earlier commit. I forgot to remove it when I reverted back to using interpolation within the strings.

@ashmaroli

ashmaroli Aug 16, 2016

Member

I agree, it was added to satisfy rubocop with an earlier commit. I forgot to remove it when I reverted back to using interpolation within the strings.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 16, 2016

Member

Looks like Rubocop doesnt want to keep it simple. 😦

Member

ashmaroli commented Aug 16, 2016

Looks like Rubocop doesnt want to keep it simple. 😦

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 16, 2016

Member

I recommend wrapping that method to disable that Rubocop check for now:

# rubocop:disable Metrics/AbcSize
def method
...
end
# rubocop:enable Metrics/AbcSize
Member

mattr- commented Aug 16, 2016

I recommend wrapping that method to disable that Rubocop check for now:

# rubocop:disable Metrics/AbcSize
def method
...
end
# rubocop:enable Metrics/AbcSize
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 16, 2016

Contributor

You'll need to squash before we can approve @ashmaroli, looks like Jekyllbot resets your approvals once you commit again, that's pretty clever and great programming by @parkr.

Contributor

envygeeks commented Aug 16, 2016

You'll need to squash before we can approve @ashmaroli, looks like Jekyllbot resets your approvals once you commit again, that's pretty clever and great programming by @parkr.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 16, 2016

Member

Squashed..
I like Jekyllbot resetting approvals. It makes sense too, as the new commit may have brought in changes not acceptable to the maintainers.
+1 to Parker for that.

Member

ashmaroli commented Aug 16, 2016

Squashed..
I like Jekyllbot resetting approvals. It makes sense too, as the new commit may have brought in changes not acceptable to the maintainers.
+1 to Parker for that.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 16, 2016

Member

LGTM. Thanks for making the updates! 🤘

Member

mattr- commented Aug 16, 2016

LGTM. Thanks for making the updates! 🤘

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 16, 2016

Member

You're welcome 😃 🤘

Member

ashmaroli commented Aug 16, 2016

You're welcome 😃 🤘

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 16, 2016

Member

There's that travis bug again.. Restart required..

Member

ashmaroli commented Aug 16, 2016

There's that travis bug again.. Restart required..

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 16, 2016

Member

LGTM. 👍 :shipit:

Member

mattr- commented Aug 16, 2016

LGTM. 👍 :shipit:

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 16, 2016

Contributor

LGTM. @jekyllbot: merge +minor

Contributor

envygeeks commented Aug 16, 2016

LGTM. @jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit b82b93c into jekyll:master Aug 16, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Approved by @mattr-. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Aug 16, 2016

@ashmaroli ashmaroli deleted the ashmaroli:colorize-embed branch Aug 18, 2016

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