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

Fix typo in Stevenson #1166

Merged
merged 1 commit into from Jun 2, 2013

Conversation

Projects
None yet
3 participants
@brainkim
Contributor

brainkim commented May 31, 2013

Before you merge or close or whatever, I took a look at Stevenson and had the following thoughts.

  1. Why is Stevenson an instantiated class versus a module that is exclusively composed of module methods? Is there any reason for Stevenson to have internal state, when any variables would be better expressed as a config/option? In other words, even if we ever wanted a different log level (and I can't imagine why we would ever want to selectively log things), the best way to implement this would be to have Stevenson collaborate with command line options or Configurations.
    Additionally, the only way to change the log level currently would be something like Stevenson.new(Stevenson.DEBUG) or Steveson.new(4), which would mean anyone who wants to use Stevenson needs to either refer to a constant which is namespaced in Stevenson, or use a plain number which isn't very descriptive. In other words, you need an intimate knowledge of the inner workings of Stevenson just to instantiate it.
    • Proposal: Turn Stevenson into a Module and make it's instance methods class/module methods
  2. "Stevenson" is a poor name. Even if it is a reference to Robert Louis Stevenson, it still doesn't indicate what the core responsibility of Stevenson is.
    • Proposal: Rename "Stevenson" to "Formatter" or "Console"
  3. Core extensions are evil. Core extensions that are found in dependencies are very evil. The fact that "red" and "yellow" are extensions of String is completely opaque to people who are looking at the code for the first time, and anyone who wants to see what it does will have a hard time finding where it's defined. Requiring a gem to do simple ANSI color formatting is overkill.
    • Proposal: Remove "colorator" from dependencies and pull in required methods for formatting.

I really like the decision to move CLI output into it's own class, but Stevenson needs work.

@brainkim

This comment has been minimized.

Contributor

brainkim commented May 31, 2013

Lemme know your thoughts and I'll push or close at your bidding.

@parkr

This comment has been minimized.

Member

parkr commented Jun 2, 2013

Hey! Thanks for your comments. I'll address them best I can below:

  1. Jekyll's logger was previously a module with class methods. We renamed it to "Stevenson" in the hopes that it would solve the inheritence problem posed by pygments.rb when pygments is required from within the Jekyll module (conflict with Ruby's stdlib Logger class). I turned it into a class in a first step toward offering users log levels in output for debugging. If something doesn't work, one could set the log_level in their _config.yml to debug and they'll get more verbose output. That is, of course, unfinished. Just haven't gotten around to doing it quite yet. By using a class, we don't need to pass in the log level every time we ask it to output something – it has that stored internally and just knows.
  2. I'm sorry you don't like the reference to the author of Jekyll & Hyde. I do, however, think neither Console nor Formatter do the job any better in describing what this class is.
  3. Core extensions don't seem nearly as sinister as you make them out to be. In the case of adding colorized output, it makes more sense to call String#red than colorize(string, :red) or ColoredString.new(string, :red) in my opinion. It's certainly simpler this way and removes any excesses here. Colorator will soon handle $TERM detection and only print color where appropriate.
@parkr

This comment has been minimized.

Member

parkr commented Jun 2, 2013

For now, I'm going to merge this fix and we can continue to discuss as appropriate. Also: @mattr-.

parkr added a commit that referenced this pull request Jun 2, 2013

Merge pull request #1166 from dracula2000/stevenson
Fix typo in Stevenson: "ERROR" only has three "R's".

@parkr parkr merged commit c0bead4 into jekyll:master Jun 2, 2013

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Jun 2, 2013

@brainkim brainkim deleted the brainkim:stevenson branch Jun 3, 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.