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 Logging and Deprecation Messaging #959

Merged
merged 10 commits into from Apr 14, 2013

Conversation

Projects
None yet
6 participants
@parkr
Member

parkr commented Apr 13, 2013

  • Centralize formatting
  • Colorize

@parkr parkr referenced this pull request Apr 13, 2013

Closed

Update CLI documentation #877

@brainkim

This comment has been minimized.

Contributor

brainkim commented on a355762 Apr 13, 2013

Heads-up: Is the :drafts and :show_drafts conversion found in normalize_options another deprecated option? Should that get moved to deprecator?

This comment has been minimized.

Member

parkr replied Apr 13, 2013

No, it's not a deprecation. It's that we wanted to allow the --drafts option, but thought the drafts config didn't represent what it meant in the configuration properly. It's a switch to turn it on, so it's converted into show_drafts: true.

This comment has been minimized.

Contributor

brainkim replied Apr 13, 2013

I see. Yeah. It's weird that we call 'em options when they go in and configurations when they come out.

This comment has been minimized.

Member

parkr replied Apr 13, 2013

That's not unusual :)

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 14, 2013

I like what I see here. There's just one thing nagging at me though. The info, warn, etc. methods don't seem to belong on Jekyll. I know that's where they need to go to make things work currently, but I'm wondering if they shouldn't hang off an object called JekyllCLI or some other such thing.

I don't think my above comment is a blocker at all, just something that bothered me a bit as I was reading this

puts "ERROR: YOUR SITE COULD NOT BE BUILT:"
puts "------------------------------------"
puts e.message
Jekyll.warn "ERROR:", "YOUR SITE COULD NOT BE BUILT:"

This comment has been minimized.

@tombell

tombell Apr 14, 2013

Contributor

Surely this should be an error call, as it is an 'ERROR'?

This comment has been minimized.

@parkr

parkr Apr 14, 2013

Member

Haha, you're right! I added the error method after I had changed this and forgot about it.

#
# Returns nothing
def error(topic, message)
$stderr.puts (Jekyll.message(topic, message)).red

This comment has been minimized.

@tombell

tombell Apr 14, 2013

Contributor

Are the brackets actually needed around Jekyll.message?

This comment has been minimized.

@parkr

parkr Apr 14, 2013

Member

Nope! I'll remove them :)

@mojombo

This comment has been minimized.

Contributor

mojombo commented Apr 14, 2013

Love it. Kind of agree with @mattr- about not having the log methods on Jekyll. They're already namespaced inside Jekyll::Logger, we could always make them class methods and just call them directly from there.

@parkr

This comment has been minimized.

Member

parkr commented Apr 14, 2013

I agree – I'll move everything back to Jekyll::Logger and call it explicitly from there.

parkr added a commit that referenced this pull request Apr 14, 2013

Merge pull request #959 from mojombo/deprecator
Refactor Logging and Deprecation Messaging

@parkr parkr merged commit 398cd63 into master Apr 14, 2013

1 check passed

default The Travis build passed
Details

@parkr parkr deleted the deprecator branch Apr 14, 2013

parkr added a commit that referenced this pull request Apr 14, 2013

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