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

Fix relative log file creation #83

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix relative log file creation #83

wants to merge 2 commits into from

Conversation

jgeiger
Copy link
Contributor

@jgeiger jgeiger commented Jun 5, 2014

Passing a relative path for the log file doesn't work on a Mac, and possibly elsewhere. If the passed in log argument is not an absolute path, it will now prepend the DAEMON_ROOT.

Also, fix rspec deprecation warnings.

Joey Geiger added 2 commits June 5, 2014 11:53
When running locally on a Mac, it seems that
passing a relative path to the log argument doesn't
create the expected log file. This will prepend the
DAEMON_ROOT to the passed in path if needed so we
get a good path.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling 21ab7a8 on jgeiger:fix_relative_log_file into 73c1318 on kennethkalmer:master.

@@ -194,6 +194,14 @@ def create_logger
end
end

def set_logger_file(log_path)
if log_path
log_path.start_with?('/') ? log_path : [DAEMON_ROOT, log_path].join('/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it may be an issue, but since I don't write or deploy to a Windows box, I have no way of testing it. My workaround for the current version is just to pass the full absolute path. Maybe a better solution is to just change the filename, which is all I really need to do anyway.

@kennethkalmer
Copy link
Owner

Windows issues aside, how about making it a little more flexible and provide users with a mechanism to outright replace the logger with their own configured instance of a logger that looks like a logger?

@marcbowes
Copy link
Collaborator

Should we just merge this as is? I'm inclined.

@kennethkalmer
Copy link
Owner

Let me take a spike at the alternative, if it goes well we can compare notes!

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.

None yet

4 participants