spdlog-rs icon indicating copy to clipboard operation
spdlog-rs copied to clipboard

Pattern-based formatter

Open Lancern opened this issue 3 years ago • 1 comments

This PR introduces pattern-based formatters.

Related issue: #10

Lancern avatar Jun 27 '22 06:06 Lancern

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

Fixes WIP

  • [x] Change the pattern template string of style ranges from {^...$} to {^...};
  • [x] Limit the names of user-defined custom patterns to match the restrictions of Rust identifiers;
  • [x] Replace - with _ in pattern names;
  • [x] Put TimeDate::tz_offset_str into a lazy closure;
  • [x] Each user-defined pattern should only be associated with one name;
  • [x] Remove single-character pattern names;
  • [x] Missing a built-in pattern for {year}-{month}-{day} (the ISO 8601 date format);
  • [x] Implement Formatter::clone_box for PatternFormatter;
  • [x] Ambiguous wording in the documentation for Pattern struct
  • [x] Get local time from the global time cacher in built-in date time patterns;
  • [x] Add a new cache value for the short year string;
  • [x] Add a pub(crate) method for Level that returns the level names;
  • [x] Do not include the column number in the output of the Loc pattern;
  • [x] ~Add P: Pattern bound to PatternFormatter<P>;~ UPDATE: Referred to this answer, bound trait only on all impl blocks, struct keeps unbounded.
  • [x] Rename ColorRange to StyleRange.

Unresolved Questions

  • [x] Re-export spdlog_macros::pattern directly from spdlog rather than wrapping it behind a macro by example;
  • [x] ~Too-long pattern names can still make the pattern template string unreadable;~ So far so good.
  • [x] Remove pattern_formatter!.

Lancern avatar Jun 30 '22 03:06 Lancern

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:

  • 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.
  • After splitting, macro pattern!("template") will return a struct Pattern<(impl PatternEntity, impl PatternEntity, ...)>, and we need a wrapper like this to prevent users from accessing the tuple, as it is implementation details.
  • In the future we may support compiling patterns at runtime, and after splitting we can add function fn Pattern::compile("template") -> Pattern<RuntimePattern> for this purpose.
  • In the current documentation, there is some confusion between the terms of Pattern, Template and Placeholder, which can be confusing for users to read.
  • This split change should not add any performance cost.

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

@Lancern What is your opinion? :)

SpriteOvO avatar Sep 18 '22 08:09 SpriteOvO

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}.

Lancern avatar Sep 18 '22 09:09 Lancern

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.

SpriteOvO avatar Sep 18 '22 09:09 SpriteOvO