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

Add GoStringer support #68

Merged
merged 1 commit into from Aug 31, 2020
Merged

Add GoStringer support #68

merged 1 commit into from Aug 31, 2020

Conversation

2opremio
Copy link
Contributor

Print values using the GoString() method (from fmt.GoStringer()) if implemented.

This gets the formatter closer the behavior of the fmt stdlib.

Print values using the `GoString()` method (from `fmt.GoStringer()`) if implemented.
@kr
Copy link
Owner

kr commented Aug 28, 2020

Hi @2opremio, thanks for doing this work! I'm a little uncertain about the purpose of this change. What problem are you running into that this would help with?

I ask because this package is meant to show the structure of the data. Calling the method GoString (or String) seems like it would often defeat the purpose.

@2opremio
Copy link
Contributor Author

I need to print large structures with long binary arrays which can be highly simplified and shortened by using GoString(). (See stellar/go#2809 and https://github.com/stellar/go/pull/2809/files#diff-5917a10b17629dda73b8fa8cd9b17d71 in particular, which is what I am trying to improve).

I was using fmts formatter (which supports GoString() in the same way I implemented it here) but I want indentation and zero-value omission (see #67) which is why I am giving your library a try and I created the PR.

GoString() (unlike String()) is supposed to print equivalent go code, so I wouldn't be too worried about it.

@2opremio
Copy link
Contributor Author

If you read GoStringers example in the Go documentation, it is precisely aimed at pretty printing.

In fact, the example prints the creation of a struct reference, instead of a pointer which is exactly what this library does automatically. This arguably hides information which could be valuable for debugging (the pointer is not printed anymore) but that drawback is minimal compared to showing the full structure by traversing the data structurel (you can always use ``%v` if you really want to see the pointers).

@2opremio
Copy link
Contributor Author

2opremio commented Aug 28, 2020

Another use case is showing constant symbol names instead of literals.

For instance:

type PetType int
const (
    Cat PetType = 0
    Dog PetType = 1
)

You can easily define a GoString() method that prints packageName.Cat or packageName.Dog instead o 0 and 1, making the output way more friendly.

@kr
Copy link
Owner

kr commented Aug 30, 2020

Those are good points! If the GoString method returns something that's not useful, then too bad I guess. Assuming it's doing its job, then it's a good fit for our purpose here.

But I think it should not be an option unless we're sure people really need both behaviors. Ideally we would figure out the best thing to do and just make it do that. And it seems like the best thing is to call GoString.

What do you think?

@2opremio
Copy link
Contributor Author

I agree, I am think we should just call GoString() (which is what the PR does).

@kr
Copy link
Owner

kr commented Aug 31, 2020

which is what the PR does

Oh yes, my apologies, I was confusing this PR with the flag in #69.

@kr kr merged commit 59b4212 into kr:main Aug 31, 2020
@kr
Copy link
Owner

kr commented Aug 31, 2020

Thanks again! This looks like it'll help make the output more readable. 🙌

@2opremio
Copy link
Contributor Author

no worries! Could you take a look at #69 ?

@2opremio 2opremio deleted the add-gostringer-support branch August 31, 2020 13:34
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.

None yet

2 participants