Add position formatter #90
Conversation
There was a problem hiding this comment.
Thanks @iRevive
I like how it looks in general! Have only a few notes on possible performance optimizations and API compatibility
| * `ThrowableFormat.Depth.Fixed` - prints N elements of a stack trace | ||
| * `ThrowableFormat.Indent.NoIndent` - prints a stack trace without indentation | ||
| * `ThrowableFormat.Indent.Fixed` - prints a stack trace prepending every line with N spaces | ||
| * [[ThrowableFormat.Depth.Full]] - prints all elements of a stack trace |
There was a problem hiding this comment.
[[]] brackets break the sbt doc (and therefore the release) unless it contains the absolute path with package, such as
io.odin.formatter.options.ThrowableFormat.Depth.Full
You can try it locally using sbt doc
| input match { | ||
| case Nil => builder | ||
| case head :: Nil => builder.append(head) | ||
| case head :: tail => loop(tail, builder.append(head.headOption.getOrElse('?')).append('.')) |
There was a problem hiding this comment.
You can save an allocation of Option object by using a simple condition if (head.isEmpty) "?" else head.head.
| } | ||
| } | ||
|
|
||
| loop(enclosure.split('.').toList, new StringBuilder).toString() |
There was a problem hiding this comment.
You can avoid bouncing over Char by using string right away, as under the hood it still allocates yet another string object.
Also, I'd stay with Array and just check its size in loop to avoid List conversion
There was a problem hiding this comment.
Also, it would make sense to move "." to some constant private val dot inside of Formatter object to avoid spawning those String instances on each call.
| * @param colorful use different color for thread name, level, position and throwable | ||
| */ | ||
| def create(throwableFormat: ThrowableFormat, colorful: Boolean): Formatter = { | ||
| def create(throwableFormat: ThrowableFormat, positionFormat: PositionFormat, colorful: Boolean): Formatter = { |
There was a problem hiding this comment.
Ooops, you're breaking already released API here. Either create overloaded method create that takes three arguments, or put it in the end of arguments list and give it a default value
There was a problem hiding this comment.
I've added overloaded methods to Formatter and circe.Formatter.
|
Looks much better, thanks for the suggestion!
|
iRevive
force-pushed
the
position-formatter
branch
from
438ce7f to
6622f56
Compare
| } | ||
| } | ||
|
|
||
| loop(enclosure.split('.'), new StringBuilder).toString() |
There was a problem hiding this comment.
Let's reuse dot here as well
There was a problem hiding this comment.
split treats string argument as a regex. The suitable options are "\\." and '.'.
Should I change it to:
loop(enclosure.split(packageSeparator), new StringBuilder).toString()
}
private val packageSeparator = "\\."
?
There was a problem hiding this comment.
Ah right, it's a special symbol. Nvm then, it's not worth it
|
Lovely |
This PR brings an additional way to configure the format of the position. (scope of #80)
Benchmarks
Master
Position format