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

Cleanup LiveReloadReactor #6607

Merged
merged 6 commits into from Dec 14, 2017

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Dec 7, 2017

some changes to the livereload reactor I withheld requesting to made before this class was merged to avoid noise in that PR..

@ashmaroli ashmaroli added this to the v3.7.0 milestone Dec 7, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 7, 2017

Member

Guys, I want to draw your attention towards the following code which is in our master now..

private
def log_error(e)
Jekyll.logger.warn(
"LiveReload experienced an error. "\
"Run with --verbose for more information."
)
Jekyll.logger.debug("LiveReload Error:", e.message)
Jekyll.logger.debug("LiveReload Error:", e.backtrace.join("\n"))
end

It doesn't seem right to me that we are going to be printing the backtrace in the debug channel when it is already crowded with build-related info..

/cc @parkr

Member

ashmaroli commented Dec 7, 2017

Guys, I want to draw your attention towards the following code which is in our master now..

private
def log_error(e)
Jekyll.logger.warn(
"LiveReload experienced an error. "\
"Run with --verbose for more information."
)
Jekyll.logger.debug("LiveReload Error:", e.message)
Jekyll.logger.debug("LiveReload Error:", e.backtrace.join("\n"))
end

It doesn't seem right to me that we are going to be printing the backtrace in the debug channel when it is already crowded with build-related info..

/cc @parkr

@ashmaroli ashmaroli changed the title from Cleanup LiveReload Reactor to Cleanup LiveReload Reactor [WIP] Dec 7, 2017

ashmaroli added some commits Dec 8, 2017

remove unnecessary ivar initialization
instance variables associated with accessors are auto-initialized with
a default value of `nil`
@DirtyF

DirtyF approved these changes Dec 8, 2017

👍 If Rubocop don't complain

@ashmaroli ashmaroli changed the title from Cleanup LiveReload Reactor [WIP] to Cleanup LiveReloadReactor Dec 11, 2017

@mattr-

mattr- approved these changes Dec 14, 2017

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Dec 14, 2017

Member

@jekyllbot: merge +dev

Member

mattr- commented Dec 14, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit acb82c9 into jekyll:master Dec 14, 2017

3 checks passed

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

@ashmaroli ashmaroli deleted the ashmaroli:reload-cleanup branch Dec 15, 2017

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