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.

Sign up for GitHub

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

Jump to bottom

Add truncate function for Go templates#30484

Merged
merged 1 commit into from
Jan 31, 2017
Merged

Add truncate function for Go templates #30484

merged 1 commit into from
Jan 31, 2017

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
tm, err := Parse(`{{shorten .}}`)
if err != nil {
t.Fatal(err)
}
Copy link
Member

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
var b bytes.Buffer
if err := tm.Execute(&b, "tupx5xzf6hvsrhnruz5cr8gwp"); err != nil {
t.Fatal(err)
}
Copy link
Member

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
want := "tupx5xzf6hvs"
if b.String() != want {
t.Fatalf("expected %s, got %s", want, b.String())
}
Copy link
Member

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
"lower": strings.ToLower,
"upper": strings.ToUpper,
"pad": padWithSpace,
"shorten": stringid.TruncateID,
Copy link
Member

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

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

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

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

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

The correct word your looking for is truncate.

@thaJeztah
Copy link
Member

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

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

👍 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

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

@vdemeester vdemeester vdemeester approved these changes

@LK4D4 LK4D4 LK4D4 approved these changes

@dnephin dnephin dnephin approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes
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