SpriteOvO / spdlog-rs

Fast, highly configurable Rust logging crate
https://crates.io/crates/spdlog-rs
Apache License 2.0
109 stars 12 forks source link

Pattern-based formatter #12

Closed Lancern closed 2 years ago

Lancern commented 2 years ago

This PR introduces pattern-based formatters.

Related issue: #10

Lancern commented 2 years ago

So here are the problems that need to be fixed and problems that are still unresolved after the first round of review:

Fixes WIP

Unresolved Questions

SpriteOvO commented 2 years ago

Looks like all problems from the first round of review have been solved! 🎉

There should be only one final question left, and I've been thinking about it. I'd like to try to split the current trait Pattern into two concepts, struct Pattern + trait PatternEntity (or PatternFlag or other name?).

The motivations:

Simply put, this split just adds a wrapper to the existing trait Pattern combination.

@Lancern What is your opinion? :)

Lancern commented 2 years ago

I'd like to try to split the current trait Pattern into two concepts, struct Pattern + trait PatternEntity (or PatternFlag or other name?).

I'd argue that this actually limits the ability to combine different patterns. An important usage is that, users can use built-in patterns as building blocks for their own patterns; and again, these user-crafted patterns can also be used as building blocks for even larger patterns. If pattern! returns a struct Pattern, it won't be able to be used for building larger patterns, unless we implement trait PatternEntity for struct Pattern. But then why we split trait Pattern in the first place?

Semantically, a Pattern is supposed to be a combination, but now a single placeholder is also considered a Pattern. A formatter with only a single placeholder should not make much sense to users.

Patterns can be freely combined does not mean patterns are combinations. A single placeholder as a pattern can also be useful; for instance, {full} or {payload}.

SpriteOvO commented 2 years ago

Yeah, you're right, I'll drop that idea.

I'll do some more checking and cleanup, and if there are no more issues this PR should be merged in the next few days.