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.

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

Helper method for java root logger #10

Merged
merged 2 commits into from
Nov 26, 2015
Merged

Conversation

dentarg
Copy link
Contributor

@dentarg dentarg commented Nov 26, 2015

The variable confused me a bit.

The variable confused me a bit.
@walro
Copy link
Contributor

walro commented Nov 26, 2015

I skipped one-lining so I could give it a descriptive name. Logger.get_logger("") makes me wonder, what the heck does that return? By binding it to a variable I understand that it returns the root_logger object.

@walro
Copy link
Contributor

walro commented Nov 26, 2015

What confused you btw? :)

@dentarg
Copy link
Contributor Author

dentarg commented Nov 26, 2015

We change some global state, with the set_level call, and I thought it was easy to interpret it as we change only change it for our root_logger variable... and then we don't use it anywhere.

@dentarg
Copy link
Contributor Author

dentarg commented Nov 26, 2015

By binding it to a variable I understand that it returns the root_logger object.

I don't know what a root_logger object so it didn't help me :-)

The better solution is probably to extract the code to a well named method in spec_helper?

@walro
Copy link
Contributor

walro commented Nov 26, 2015

Yeah, wrapping it up in a method would improve it a bit. But I don't agree with your initial change.

Just because you don't know what something is does not mean that you should remove them.

See https://docs.oracle.com/javase/7/docs/api/java/util/logging/LogManager.html for reference (search for root)

@dentarg
Copy link
Contributor Author

dentarg commented Nov 26, 2015

@walro WDYT?

root_logger = Logger.get_logger("")

# The first handler is by default the console
root_logger.get_handlers.first.set_level(level)
Copy link
Contributor

Choose a reason for hiding this comment

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

hihi now you can oneline since you have descriptive method name :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the comment and with this it's slightly more clear where it applies :)

@dentarg dentarg changed the title No need for a variable Helper method for java root logger Nov 26, 2015
@walro
Copy link
Contributor

walro commented Nov 26, 2015

👍

@@ -1,8 +1,5 @@
require "spec_helper"

java_import java.util.logging.Logger
java_import java.util.logging.Level
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should leave Level here, since we use it below. Suppose it's implicity added by the spec_helper now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, all specs use spec_helper, so I don't think it's needed, we will just be repeating ourselves

dentarg added a commit that referenced this pull request Nov 26, 2015
Helper method for java root logger
@dentarg dentarg merged commit ece9584 into master Nov 26, 2015
@dentarg dentarg deleted the minor-spec-refactoring branch November 26, 2015 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants