Extract text package from gh #69

samcoe · 2022-08-25T12:29:19Z

This PR extracts the text package from gh. It also includes FuzzyAgo and FuzzyAgoAbbr from the utils package because they seemed to fit in well here.

cc cli/cli#5544

samcoe · 2022-08-25T12:30:52Z

pkg/template/template.go

@@ -207,43 +206,16 @@ func timeAgo(ago time.Duration) string {
 		return "just now"


Kind of weird that we don't use FuzzyAgo instead of this entire function. Perhaps we should be? The current functionality was brought over from gh.

pkg/text/text.go

pkg/text/text_test.go

samcoe

I feel pretty good about this extraction, only thing I am a bit concerned about is the clarity of the comments. Any feedback there is appreciated.

AmEarth

text-package

mislav

I would vote to remove most of the functions there and focus on just the handful of ones that we want to control the implementation of, such as ANSI-codes aware implementations of measuring display width, truncation, and indentation.

Unlike in Ruby or JavaScript, offering a grab-bag of tiny utilities as a Go library isn't what most Go developers want. In fact, if they need some of these utilities in their own programs, most Go developers might choose to just inline them there instead of adding a dependency. Of course, gh extension authors will already depend on the go-gh repository anyway, but at least we can reduce the surface of the APIs we offer.

pkg/text/text.go

mislav · 2022-09-12T16:26:33Z

pkg/text/text.go

+}
+
+// Pluralize returns a concatenated string with num and the plural form of thing if necessary.
+func Pluralize(num int, thing string) string {


I feel that such simplistic pluralization implementation should best be inlined in people's extensions (if needed at all) instead of being exposed from go-gh

I agree that it is simplistic, but it is necessary for FuzzyAgo and also used independently of that in gh so if it isn't exposed here then it will need to be duplicated in gh. Going to leave it in for now.

pkg/text/text.go

samcoe

@mislav Thanks for the review. I made the requested changes if you wouldn't mind giving it another look 🙇

samcoe · 2022-09-13T07:45:08Z

pkg/text/text.go

+}
+
+// Pluralize returns a concatenated string with num and the plural form of thing if necessary.
+func Pluralize(num int, thing string) string {


I agree that it is simplistic, but it is necessary for FuzzyAgo and also used independently of that in gh so if it isn't exposed here then it will need to be duplicated in gh. Going to leave it in for now.

mislav

Thank you! 🚀

samcoe self-assigned this Aug 25, 2022