Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sure no trailing spaces #100

Merged
merged 3 commits into from
Jan 15, 2015
Merged

make sure no trailing spaces #100

merged 3 commits into from
Jan 15, 2015

Conversation

crquan
Copy link

@crquan crquan commented Jan 4, 2015

Closes #99 text log should not have trailing spaces.

just reorganize the patch into 3 commits:
03377c6 rename f.appendKeyValue to printKeyValue
a243bba share common calling path in printKeyValue
dcbe8d6 make sure no leading or trailing spaces

In benchmark testing, it increased ~300 ns/op in SmallTextFormatter,
that should be due to the extra slice operation [1:];
in LargeTextFormatter it reduced ~700 ns/op;

BenchmarkSmallTextFormatter       200000             10683 ns/op           8.14 MB/s
BenchmarkLargeTextFormatter        30000             43537 ns/op           6.59 MB/s
BenchmarkSmallTextFormatter       200000             10996 ns/op           7.82 MB/s
BenchmarkLargeTextFormatter        30000             42862 ns/op           6.67 MB/s

Signed-off-by: Derek Che drc@yahoo-inc.com

@sirupsen
Copy link
Owner

sirupsen commented Jan 4, 2015

I think trimSpaces is heavy handed, rather you should be able to pass an argument to appendKeyValue to remove the space, or always return b[0..len(b)-2]—the latter seems simpler, I prefer that one since there'll always be only one empty space. A comment would be appropriate too, since it's not obvious why that is.

Thanks for working on this!

@crquan
Copy link
Author

crquan commented Jan 4, 2015

or maybe code like this, just remove one possible space,

        if byts[len(byts)-1] == ' ' {
                byts = byts[:len(byts)-1]
        }

@crquan
Copy link
Author

crquan commented Jan 4, 2015

I have another TestColorPrint which tests how many spaces can the printColored func make: there could be many of them:

=== RUN TestColorPrint
"\x1b[31mPANI\x1b[0m[0000] msg content                                  \n"

So it's better to run in a loop:

for byts[len(byts)-1] == ' ' {
    byts = byts[:len(byts)-1]
}

or call bytes.TrimRightFunc:

byts = bytes.TrimRightFunc(byts, unicode.IsSpace)

Derek Che added 2 commits January 3, 2015 23:56
printKeyValue is working similar like printColored, not using
any fields of TextFormatter, should be a util func instead of
a method of TextFormatter.

Signed-off-by: Derek Che <drc@yahoo-inc.com>
Signed-off-by: Derek Che <drc@yahoo-inc.com>
This changed printColored and printKeyValue to print in same way
with prefix space instead of trailing space, to make it easier
to slice out when returning in Format;
The test cases are to make sure msg formartting doesn't include
leading or trailing spaces;

Closes sirupsen#99

Signed-off-by: Derek Che <drc@yahoo-inc.com>
@crquan
Copy link
Author

crquan commented Jan 14, 2015

@sirupsen do you have an update on this? I'm looking into this because docker code is referencing here, and every docker ci has log lines ends a space.
https://ci.dockerproject.com/github.com/docker/docker/commit/e00e161f4738cede19b37f4cd33ae643863ea570?branch=10081-fix-renaming

@sirupsen
Copy link
Owner

Can you squash? Otherwise looks good

@crquan
Copy link
Author

crquan commented Jan 15, 2015

this was initially in a single patch but I have split it into 3 separate to make each one's intention be clear; now to squash them is surely easy, but why does here prefer a squashed single commit? or do you have any worries on number of commits?

sirupsen added a commit that referenced this pull request Jan 15, 2015
make sure no trailing spaces
@sirupsen sirupsen merged commit c6a969a into sirupsen:master Jan 15, 2015
sirupsen added a commit that referenced this pull request Jan 15, 2015
This reverts commit c6a969a, reversing
changes made to 3c5b048.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

text log should remove trailing space
3 participants