Set log level to debug when verbose flag is set #3665

Merged
merged 3 commits into from Apr 20, 2015

Conversation

Projects
None yet
4 participants
@purp
Contributor

purp commented Apr 19, 2015

Adds Jekyll::LogAdapter#adjust_verbosity which ensures that --quiet always wins. Also refactors Jekyll::Configuration minorly to dry up the #source, #quiet?, and new #verbose? methods.

Tests included; all tests pass.

purp added some commits Apr 18, 2015

Set logging to debug when verbose flag is set
Adds Jekyll::LogAdapter#adjust_verbosity which ensures that --quiet
always wins.
DRY config value fetching
Adds #get_config_value_with_override, refactoring the three fetch
methods to use it.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 19, 2015

Is there any reason not to just use fetch here? I'm not against this version just asking.

Is there any reason not to just use fetch here? I'm not against this version just asking.

This comment has been minimized.

Show comment
Hide comment
@purp

purp Apr 19, 2015

Owner

I didn't really think on it much; I was just carrying the previous implementation into this refactor. I don't use fetch very often myself, so it didn't occur to me. I'd guess maybe because fetch throws an error by default and the original two implementations (#source and #quiet?) didn't want that behavior? But I'd just be guessing.

I think the fetch implementation would have to look like:

override.fetch(config_key) {|key| self.fetch(key, DEFAULTS[key])}

... which reads as less intuitive to my eye. I'd argue that the implementation above is more explicit and straightforward when future-me is awake in the wee hours and trying to divine intent. first_a || then_b || then_c

Very open to alternative suggestions. Not elegant, but seemed better than adding the same pattern a third time.

Owner

purp replied Apr 19, 2015

I didn't really think on it much; I was just carrying the previous implementation into this refactor. I don't use fetch very often myself, so it didn't occur to me. I'd guess maybe because fetch throws an error by default and the original two implementations (#source and #quiet?) didn't want that behavior? But I'd just be guessing.

I think the fetch implementation would have to look like:

override.fetch(config_key) {|key| self.fetch(key, DEFAULTS[key])}

... which reads as less intuitive to my eye. I'd argue that the implementation above is more explicit and straightforward when future-me is awake in the wee hours and trying to divine intent. first_a || then_b || then_c

Very open to alternative suggestions. Not elegant, but seemed better than adding the same pattern a third time.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 19, 2015

The readability is a fair point to me and the only point that needs justification. I'm sold!

The readability is a fair point to me and the only point that needs justification. I'm sold!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 19, 2015

Is there a good reason to duplicate this test? If you have to test twice on something like this it seems to me like there might be a problem with the code above.

Is there a good reason to duplicate this test? If you have to test twice on something like this it seems to me like there might be a problem with the code above.

This comment has been minimized.

Show comment
Hide comment
@purp

purp Apr 19, 2015

Owner

I think I'm not following your meaning. The two tests above and the one below intend to prove three separate assertions:

  • setting --quiet logs at :error
  • setting --verbose logs at :debug
  • setting both is the same as setting only --quiet

I tend to favor one basic assert per test, so three tests. I suppose I could combine the two, adjust the verbosity twice, and assert twice, if that's what you're getting at. Otherwise, could you say more?

Owner

purp replied Apr 19, 2015

I think I'm not following your meaning. The two tests above and the one below intend to prove three separate assertions:

  • setting --quiet logs at :error
  • setting --verbose logs at :debug
  • setting both is the same as setting only --quiet

I tend to favor one basic assert per test, so three tests. I suppose I could combine the two, adjust the verbosity twice, and assert twice, if that's what you're getting at. Otherwise, could you say more?

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 19, 2015

I reread the tests against the code without a diff and agree. Ignore my comment.

I reread the tests against the code without a diff and agree. Ignore my comment.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 19, 2015

Try a regexp here, because this test couples tightly to the source and isn't refactor friendly IMO.

Try a regexp here, because this test couples tightly to the source and isn't refactor friendly IMO.

This comment has been minimized.

Show comment
Hide comment
@purp

purp Apr 19, 2015

Owner

Fair enough. I'll drop the with() portion as it's not critical to the assertion that #adjust_verbosity sends a debug log.

Owner

purp replied Apr 19, 2015

Fair enough. I'll drop the with() portion as it's not critical to the assertion that #adjust_verbosity sends a debug log.

parkr added a commit that referenced this pull request Apr 20, 2015

@parkr parkr merged commit 6ce345b into jekyll:master Apr 20, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 20, 2015

Member

Rock solid. 🤘

Member

parkr commented Apr 20, 2015

Rock solid. 🤘

parkr added a commit that referenced this pull request Apr 20, 2015

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