addr-rs / addr

Parse domain names reliably and quickly in Rust
MIT License
51 stars 12 forks source link

Refactor to not use `slog_scope` #1

Closed dpc closed 7 years ago

dpc commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:-

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.