Skip to content

Conversation

jvoisin
Copy link
Collaborator

@jvoisin jvoisin commented Oct 12, 2025

Instead of having as structure with every possible type as member, use a single any. The type is determined by the default value, reducing the amount of copy/paste, code complexity, as well as memory consumption.

This PR is sent as a draft to gather feedback on the idea before moving all the different types to this new parsing method.

Instead of having as structure with every possible type as member, use a single
`any`. The type is determined by the default value, reducing the amount of
copy/paste.

This PR is sent as a draft to gather feedback before moving all the different
types to this new parsing method.
@jvoisin jvoisin requested a review from fguillot October 12, 2025 19:50
@jvoisin
Copy link
Collaborator Author

jvoisin commented Oct 15, 2025

What's your opinion on the idea @fguillot ?

@fguillot
Copy link
Member

What's your opinion on the idea @fguillot ?

The entire point of the current implementation was to avoid using reflection and type assertion. I haven't verified everything, but I can see few regressions in your changes if you compare the output of go run main.go -config-dump.

@jvoisin
Copy link
Collaborator Author

jvoisin commented Oct 18, 2025

This is only a draft, so regressions are to be expected. Take it as a proof-of-concept, not something that is mergeable as is.

As for the usage of reflection, it's only used for logging/debugging (those shouldn't be present in the final PR), and never to guess types: the type of every configuration option is know at compile-time. While type assertions are indeed used, notice that the anyToStr and anyToBool functions are panic'ing if the assertion fails, conveying that those won't fail at runtime.

Moreover, it allows to get rid of all the Parsed*Value members, all the *Type configValueType constants, simplify the options map from at least 4 values (including a duplicate) to often only 1, and finally it simplifies a bit the implementation of parseLine.

I wrote this PR while working on polishing #3809, as I found it tedious to manipulate configuration options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants