Skip to content

Conversation

@bombsimon
Copy link
Contributor

Add support to always show types, even for built-in types such as string, int, int32 etc. This will also show type for type aliases of such types.


I find myself quite often wanting to know the type even for built-in values, e.g. the exact type of integer or the aliased type for a type alias. I also miss this when dumping types such as []any or map[string]any.

I don't know if this should actually be feature gated with a feature flag like this or if I should've filed an issue and discussed this as a potential default first instead, but this was simple enough to test so here's just an example/RFC on how to let the user print all types.

Before (current)

image

After (using WithShowAllTypes)

image

Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

The feature is great, I would make it thw default behavior.

But let's @Akkadius feedback

@bombsimon
Copy link
Contributor Author

I think if this is of interest it should likely be rewritten in a better way so we can re-use the type inference for structs when we list maps or slices. Right now we can see built-in types but a map[string]SomeType would just be listed as map[string]struct which isn't great.

Also I wonder if we should only list the type of the keys on each row for maps or something else? 🤔

Also I agree this should just be default and not put behind a flag which just adds code and noise but I'll leave this PR as is until it's confirmed this is wanted.

@Akkadius
Copy link
Member

@bombsimon thank you for contributing.

I believe this should be default behavior, as such we should remove the option. Sane defaults, less knobs, simple.

There's a lot of duplicated code as well and I think things can be encapsulated a bit better. Rebase, address comments and we should be good to go.

@bombsimon bombsimon force-pushed the feat/always-show-types branch 2 times, most recently from 41cd869 to 22ac1af Compare August 11, 2025 22:03
@bombsimon
Copy link
Contributor Author

I took a new stab at this now which should be more robust and handle types for maps and slices and recursive types etc.

I tried to do as little change as possible but there are definitely some places that are recursive that wasn't as easy, maybe it would be better to compute type completely separate from printing it but that feels like a too big re-design.

I also dropped the visited map to printValues since it wasn't used, maybe that was a bad idea?

@cmilesdev
Copy link

@bombsimon I'm so sorry a response has taken so long. I've had a lot of life events occur in the past few months.

Looks like we have some test issues to resolve yet. Happy to help get this one to the finish line once we're sorted.

@bombsimon
Copy link
Contributor Author

@bombsimon I'm so sorry a response has taken so long. I've had a lot of life events occur in the past few months.

No worries, life comes first!

Looks like we have some test issues to resolve yet. Happy to help get this one to the finish line once we're sorted.

It's linting failing and I didn't want to change either the linter settings nor the actual code. It's failing due to complexity of the already huge printValue function. I can give it a go to try to refactor but didn't want to do it for this feature. Alternatively we can just change the setting for the linter or ignore it (completely or for this function only) but I leave that up to you.

Let me know which approach you prefer!

@cmilesdev
Copy link

@bombsimon I'm so sorry a response has taken so long. I've had a lot of life events occur in the past few months.

No worries, life comes first!

Looks like we have some test issues to resolve yet. Happy to help get this one to the finish line once we're sorted.

It's linting failing and I didn't want to change either the linter settings nor the actual code. It's failing due to complexity of the already huge printValue function. I can give it a go to try to refactor but didn't want to do it for this feature. Alternatively we can just change the setting for the linter or ignore it (completely or for this function only) but I leave that up to you.

Let me know which approach you prefer!

Feel free to adjust the linter settings

bombsimon and others added 4 commits October 21, 2025 20:57
Add support to always show types, even for built-in types such as
`string`, `int`, `int32` etc. This will also show type for type aliases
of such types.
@bombsimon bombsimon force-pushed the feat/always-show-types branch from 22ac1af to 00db455 Compare October 21, 2025 19:03
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@bombsimon
Copy link
Contributor Author

Fixed the rebase and updated the config! Feel free to give this an extra go or manual test to confirm it's according to your expectations.

Also I'm curious about the golangci-lint setup in the project. I didn't investigate too much, but it passes in CI but locally I get a bunch of revive reports with the same conifg.

› golangci-lint --version
golangci-lint has version 2.1.6 built with go1.24.2 from eabc2638 on 2025-05-04T15:41:19Z

› golangci-lint run --output.text.path /dev/null
[...]
59 issues:
* revive: 59

They're all of type unhandled-error and most of them from fmt.Fprintf.

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.

4 participants