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

Add Android sink #130

Merged
merged 1 commit into from
Sep 14, 2015
Merged

Add Android sink #130

merged 1 commit into from
Sep 14, 2015

Conversation

ruslo
Copy link
Contributor

@ruslo ruslo commented Sep 10, 2015

No description provided.

case spdlog::level::critical: return ANDROID_LOG_FATAL;
case spdlog::level::alert: return ANDROID_LOG_FATAL;
case spdlog::level::emerg: return ANDROID_LOG_FATAL;
case spdlog::level::off: throw spdlog_ex("Unexpected off");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about what is this level about, so just throw exception. Let me know if it's not correct.

Copy link
Owner

Choose a reason for hiding this comment

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

Off means no logging. It should not throw, just ignore the log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Off means no logging. It should not throw, just ignore the log

Should we return from _sink_it (i.e. if (level == off) return) or check assert(level != off)?

Copy link
Owner

Choose a reason for hiding this comment

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

Just return i think

@@ -31,6 +31,7 @@
#include "../sinks/file_sinks.h"
#include "../sinks/stdout_sinks.h"
#include "../sinks/syslog_sink.h"
#include "../sinks/android_sink.h"
Copy link
Owner

Choose a reason for hiding this comment

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

No need to include this file. It is not used from splog_imple.h as far as i can tell

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 was thinking that the only file user need to include to work with library is #include "spdlog/spdlog.h" so I guess we need to include sinks/android_sink.h somewhere internally. I've just found other sinks and add android one there.

Copy link
Owner

Choose a reason for hiding this comment

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

No. They are included because they are used in this file. If android is not used in spdlog_impl.h, it should not included

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabime Okay, so to clarify: I'm removing this line and it means that if user want to use android sink he should include header manually, like #include <spdlog/sinks/android_sink.h. Guess it would be nice to document it somewhere (?)

Copy link
Owner

Choose a reason for hiding this comment

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

@ruslo Right.

@ruslo
Copy link
Contributor Author

ruslo commented Sep 14, 2015

@gabime updated

gabime added a commit that referenced this pull request Sep 14, 2015
@gabime gabime merged commit b2e948c into gabime:master Sep 14, 2015
@ruslo ruslo deleted the android.sink branch September 15, 2015 10:44
This pull request was closed.
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