align message level

[Kikobeats]

Ideally would be aligned calculating the level with more characters as reference and adding the spaces necessary for each level to be align.

[indexzero]

Not opposed. Could you add a test for this?

[Kikobeats]

I added tests, but I'm doing something wrong with the testing library... what happens? is my first time with vows.

btw need documentation as well

[paulhroth]

The problem is that vows runs the test cases asynchronously which is causing race conditions with the stdMocks module. You can use addBatch to add a new batch for each test with stdMocks -- batches are run synchronously (with respect to each other). See my PR #630 for an example.

[paulhroth]

@Kikobeats I fixed your tests and opened a PR over at your fork. If you merge it this PR will pass the CI build :) Hope your feature gets in, it'll be really useful!

[Kikobeats]

thanks @paulhroth!
tests passing @indexzero!

[paulhroth]

No problem! Out of interest, I dig some digging into the codebase regarding this feature. Here's what I found:

There's a padLevels option for Loggers which already exists to solve this problem. It prepends an appropriate number of spaces (based on the length of the logger's longest level, and the level being logged) to the msg, thereby aligning it.

However padLevels is not perfect (see discussion on issue #352). One complaint is that it doesn't align labels nicely, because those are added to the output in common.log before the space-prepended msg. For this same reason, the '\t' from this PR actually breaks alignment if both your align and padLevels are enabled.

Your PR has the advantage of nicely aligning labels, and it's also Transport specific which seems better than being Logger specific to me. The downside is that for custom levels of varying lengths, adding a simple '\t' will not properly align the logs. Fixing this is challenging to do in a non-hacky way because of Winston's current architecture (see this rejected PR #431).

It could be argued that the correct solution is to either use padLevels (which is currently undocumented, and probably shouldn't be), or a custom formatter if padLevels doesn't fit your needs. I personally think this PR is still useful as its outcome in the general case (with non-custom levels) is what users have asked for in #352, and it can be implemented without the unwanted style requirements of a PR like #431.

Screenshots:

Your PR formats labels nicely, but doesn't handle level names of varying length:

The Logger's padLevels option handles level names of varying length, but doesn't format labels nicely:

When both are on, alignment is broken by the tab character before the already space-prepended msg:

tl;dr: This is very similar to padLevels, but has the advantages of formatting labels better and being Transport specific, and the disadvantage of not properly handling level names of varying length. IMO It could still be useful to add, as long as users are made aware that it is incompatible with the Logger's padLevels option, and that it doesn't have any logic for level names of varying length.

Suggestion: Maybe align could also accept a number indicating up-to how many spaces to pad (i.e., the length of the longest level). This would require the user to manually enter this, which isn't great, but it avoids the architectural problems involved with automating it, and removes the one downside this PR has (imo).

I hope this information can help you and @indexzero in going forward with this PR!

[paulhroth]

@Kikobeats I'd also recommend aligning only when showLevel is true or there's a custom timestamp, because otherwise you don't need a tab at the beginning of the line.

[Kikobeats]

time to merge?

[indexzero]

Will look at getting this merged into 2.0.0

[Kikobeats]

👍

[indexzero]

This has been cherry-picked and will be released in [email protected] later today.