Skip to content

Conversation

@dpc
Copy link

@dpc dpc commented Jan 15, 2017

Hi, author of slog here.

First of all, thanks for using slog. I sometimes visit reverse dependencies of it, and checks what's going on.

This patch changes the API (breaking change) to take the Logger during init and most importantly not use slog_scope to set the global handler.

Generally accepting Logger manually gives the user of a library maximum of flexibility. If the app is using slog_scope it can always pass slog_scope::logger(), if not it can pass the manual logger it has. Generally libraries should not be using slog_scope (IMO) as it gets in the way of the application that are using slog_scope, might want to set their own handler etc.

@dpc
Copy link
Author

dpc commented Jan 15, 2017

On the side-note, I think using a global function in the API (init and get) is rather non-idiomatic (as using any global instances etc). WhileI guess most apps would not need multiple psl instances, it seems to me that letting the user create the Psl object and then call get() on it is the most flexible way. The user can store it globally if it wants to. But it's just my opinion, feel free to ignore it (as the PR) if you disagree.

@rushmorem rushmorem merged commit 6a0e0a8 into addr-rs:master Jan 15, 2017
@rushmorem
Copy link
Collaborator

I have merged the pull request. Thank you!

You are right that globals are not idiomatic in Rust. However, like everything else, they have their advantages and disadvantages. In some use cases, those advantages outweigh the disadvantages by far (especially in Rust). That's when I believe using them makes sense.

IMHO, slog_scope is just as useful to libraries. Since this library is so small and we only have one struct, Cache it doesn't make much difference. I usually prefer slog_scope for the following reasons:-

  • I don't have to pass around the logger everywhere. This is a HUGE win for user experience (again especially in Rust). This becomes more compelling as the number of your custom datatypes grow. Passing objects around everywhere is not only tiring, it can take a toll on your productivity once you start fighting the borrow checker. For examples of real world problems, please see some of the comments in this Tokio pull request.
  • If someone was already using slog_scope in their binary, they didn't have to configure this library separately, it would work out of the box.

In a nutshell, it boils down to a better user experience. I'm aware that the current implementation only supports one list. However, this is something I have already considered and plan to address without impacting this user experience.

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