Skip to content

Conversation

@ccoVeille
Copy link
Contributor

@ccoVeille ccoVeille commented Jul 8, 2025

supersedes #23 and #24

Fix #34

@codecov
Copy link

codecov bot commented Jul 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

}
val.Recursive.Self = val.Recursive // cycle

Dump(val)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure why this one was there, maybe it something you forgot or wanted to keep, let me know

Copy link
Member

Choose a reason for hiding this comment

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

It was for manual visual testing, which I'd prefer to keep in

Copy link
Member

@Akkadius Akkadius left a comment

Choose a reason for hiding this comment

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

Generally looks good, looking forward to the fixes. Some comments on naming consistency mainly

godump.go Outdated
}

// format applies the configured formatter to the string with the given color code.
func (d *Dumper) format(code, str string) string {
Copy link
Member

Choose a reason for hiding this comment

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

"format" is ambiguous and could have many implied meanings - I'd very much prefer to keep this as "colorize"

}
return true
}

Copy link
Member

Choose a reason for hiding this comment

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

Doc comment

godump.go Outdated
// It defaults to [runtime.Caller], it is here to be overridden for testing purposes.
callerFn func(skip int) (uintptr, string, int, bool)

formatter Colorizer
Copy link
Member

Choose a reason for hiding this comment

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

Similar here, colorizer

godump.go Outdated
// htmlColorize colorizes the string using HTML span tags.
func htmlColorize(code, str string) string {
// formatHTML colorizes the string using HTML span tags.
func formatHTML(code, str string) string {
Copy link
Member

Choose a reason for hiding this comment

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

colorizeHTML

godump.go Outdated
return str
}
// formatANSIColor colorizes the string using ANSI escape codes.
func formatANSIColor(code, str string) string {
Copy link
Member

Choose a reason for hiding this comment

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

This adds Color as a qualifier to "format" as well when it could just be "colorizeANSI"

godump.go Outdated

// colorize is the default colorizer function.
var colorize Colorizer = ansiColorize // default
func formatNoColor(code, str string) string {
Copy link
Member

Choose a reason for hiding this comment

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

colorizeNone or colorizeUnstyled

@ccoVeille ccoVeille force-pushed the refactor branch 2 times, most recently from f73a23d to 5d5f9cb Compare July 8, 2025 07:15
@ccoVeille
Copy link
Contributor Author

Could you review and merge ?

I'm a bit afraid newly opened PR could cause too many conflicts if code keep diverging.

Example:

@ccoVeille ccoVeille force-pushed the refactor branch 2 times, most recently from a624abb to ce8de10 Compare July 25, 2025 16:00
@resoliwan
Copy link

I hope this PR is merged soon, as I’ve also experienced race conditions.

Thank you in advance to @ccoVeille and @Akkadius for addressing these issues.

@Akkadius Akkadius merged commit 2b54839 into goforj:main Aug 8, 2025
6 checks passed
@ccoVeille ccoVeille deleted the refactor branch August 8, 2025 23:15
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.

Global colorize and enableColor variables cause thread safety issues in DumpHTML

3 participants