Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use SLF4J loggers and add support for named hierarchies #115

Closed
wants to merge 5 commits into from
Closed

Use SLF4J loggers and add support for named hierarchies #115

wants to merge 5 commits into from

Conversation

arielvalentin
Copy link

Our attempt to address issues #110 #111

@colinsurprenant
Copy link
Owner

LGTM. reviewing this I noticed I created the Loggable module to try and DRY this up a bit but for some reason, this was never completed and Loggable is unused.

If you feel like completing this that would be great, otherwise I'll merge this and we can create an issue to not forget to complete the Loggable thing.

@arielvalentin
Copy link
Author

I will give it a shot

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling fde7148 on arielvalentin:update-logging-110-111 into 3f711ed on colinsurprenant:master.

@arielvalentin
Copy link
Author

I replaced the usage with Loggable module. I didn't think to write additional tests for it. Is that something you would like?

colinsurprenant
colinsurprenant reviewed Aug 7, 2014
View reviewed changes
lib/red_storm/dsl/topology.rb
@@ -102,7 +102,7 @@ def new_instance
end

def self.log
@log ||= Java::OrgApacheLog4j::Logger.getLogger(self.name)
@log ||= Java::OrgSlf4j::LoggerFactory.get_logger(self.name.gsub(/::/,'.'))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use Loggable here too?

@colinsurprenant
Copy link
Owner

good stuff, thanks. just curious per my inline comment why not use Loggable in the Topology class too?

@arielvalentin
Copy link
Author

Oops!! I'll fix that.

Thanks,

Ariel

Sent from my mobile device. Please excuse any errors.

On Aug 7, 2014, at 10:45 AM, Colin Surprenant notifications@github.com wrote:

good stuff, thanks. just curious per my inline comment why not use Loggable in the Topology class too?

—
Reply to this email directly or view it on GitHub.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) when pulling 02c79c9 on arielvalentin:update-logging-110-111 into 3f711ed on colinsurprenant:master.

rtyler pushed a commit to jruby-gradle/redstorm that referenced this pull request Mar 17, 2015
Merge remote-tracking branch 'colinsurprenant/pr/115'
fec511c 
Merging in colinsurprenant#115

Conflicts:
	.gitignore
	lib/red_storm/dsl/bolt.rb
	lib/red_storm/dsl/spout.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arielvalentin @colinsurprenant @coveralls