X Tutup
Skip to content

Add truncate function for Go templates#30484

Merged
vdemeester merged 1 commit intomoby:masterfrom
yongtang:28199-shorten
Jan 31, 2017
Merged

Add truncate function for Go templates#30484
vdemeester merged 1 commit intomoby:masterfrom
yongtang:28199-shorten

Conversation

@yongtang
Copy link
Member

@yongtang yongtang commented Jan 26, 2017 •
edited
Loading

This fix is part of the discussion in #28199 about using truncate to replace --no-trunc.

As part of the fix, a new function truncate has been added for Go templates so that it is possible to use

docker stack services --format "{{truncate .ID 5}}: {{.Mode}} {{.Replicas}}"

to show truncated ID (limited by length).

A unit test has been added.

This fix is related to #28199.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang yongtang mentioned this pull request Jan 26, 2017
Merged
vdemeester
vdemeester reviewed Jan 27, 2017
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design LGTM 🐮
/cc @thaJeztah @dnephin @cpuguy83 @tiborvass @AkihiroSuda @runcom @icecrime

@dnephin
Copy link
Member

dnephin commented Jan 27, 2017

LGTM

dnephin
dnephin reviewed Jan 27, 2017
View reviewed changes
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests could use the assertion helpers. I think it makes them a lot easier to read when they are shorter.

The error messages are also consistent that way, and include things like "Expected no error, but got: ..."

pkg/templates/templates_test.go Outdated
Copy link
Member

@dnephin dnephin Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

assert.NilError(t, err)

pkg/templates/templates_test.go Outdated
Copy link
Member

@dnephin dnephin Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

assert.NilError(t, tm.Execute(&b, "tupx5xzf6hvsrhnruz5cr8gwp"))

pkg/templates/templates_test.go Outdated
Copy link
Member

@dnephin dnephin Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

assert.Equal(t, b.String(), "tupx5xzf6hvs")

cpuguy83
cpuguy83 requested changes Jan 27, 2017
View reviewed changes
pkg/templates/templates.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this works on any string, I think it should take an integer argument to shorten to a certain length

Copy link
Member

@cpuguy83 cpuguy83 Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not to say that TruncateID should take an argument ;)

Copy link
Member

@vdemeester vdemeester Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point 👍

@yongtang
Copy link
Member Author

yongtang commented Jan 27, 2017

Thanks @dnephin the PR has been updated with unit tests using assertion helpers. Other tests in pkg/templates/templates_test.go has also been updated to use it. (not necessarily related to shorten but the change is small enough).

@yongtang
Copy link
Member Author

yongtang commented Jan 27, 2017

Thanks @cpuguy83 @vdemeester for the comment. Now shorten takes the int for the max length of the truncation. Please take a look.

cpuguy83
cpuguy83 approved these changes Jan 27, 2017
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dnephin
dnephin approved these changes Jan 27, 2017
View reviewed changes
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevvooe
Copy link
Contributor

stevvooe commented Jan 27, 2017

The correct word your looking for is truncate.

@thaJeztah
Copy link
Member

thaJeztah commented Jan 28, 2017

I agree that truncate would be better; it's also consistent with the current --no-trunc flag, so easier to explain that they relate.

Moving it back to design for a bit

@yongtang
Copy link
Member Author

yongtang commented Jan 28, 2017

We currently have

	"json":
	"split"
	"join"
	"title"
	"lower"
	"upper"
	"pad"

I am thinking maybe we could consider trunc?
It seems truncate is a litter long compared with other functions. Though either is fine for me.

@yongtang yongtang mentioned this pull request Jan 29, 2017
Merged
@tiborvass
Copy link
Contributor

tiborvass commented Jan 30, 2017

👍 for truncate.

@yongtang
Add truncate function for Go templates
7fa8d5e 
This fix is part of the discussion in 28199 about using
`truncate` to replace `--no-trunc`.

As part of the fix, a new function `truncate` has been
added for Go templates so that it is possible to use
```
docker stack services --format "{{truncate .ID 5}}: {{.Mode}} {{.Replicas}}"
```
to show truncated ID.

A unit test has been added.

This fix is related to 28199.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang changed the title Add shorten function for Go templates Add truncate function for Go templates Jan 30, 2017
@yongtang
Copy link
Member Author

yongtang commented Jan 30, 2017

Thanks all for the review. The PR has been updated with truncate. The title and description of the PR has also been updated. Please take a look.

LK4D4
LK4D4 approved these changes Jan 31, 2017
View reviewed changes
Copy link
Contributor

@LK4D4 LK4D4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 31, 2017

@cpuguy83 @dnephin @thaJeztah @vdemeester feel free to merge.

vdemeester
vdemeester approved these changes Jan 31, 2017
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@vdemeester vdemeester merged commit 818ef47 into moby:master Jan 31, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 31, 2017
@yongtang yongtang deleted the 28199-shorten branch January 31, 2017 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

+3 more reviewers

@vdemeester vdemeester vdemeester approved these changes

@LK4D4 LK4D4 LK4D4 approved these changes

@dnephin dnephin dnephin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

impact/changelog impact/cli status/2-code-review

Projects

None yet

Milestone

17.04.0

Development

Successfully merging this pull request may close these issues.

9 participants

@yongtang @dnephin @stevvooe @thaJeztah @tiborvass @LK4D4 @vdemeester @cpuguy83 @GordonTheTurtle
X Tutup