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 option to encode value keys with '.' separators #229

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gKits
Copy link

@gKits gKits commented Nov 28, 2024

Description

The Encoder struct has now a method called ActivateKeySeparation.
When this option is activated the keys of the values map[string][]string
will be the full path containing all names of the nested struct fields separated
by a period (.).

This also fixes the bug referenced in #228 that it is currently not possible to
decode values into structs with nested fields after encoding said structs.

I'm not sure about the naming so I'm open for suggestions if someone has better ideas.

Example

f := Foo{
    X: "hello",
    Y: Bar{
        A: "world",
        B: true,
    },
}

vals := map[string][]string{}
encoder := schema.NewEncoder()
encoder.ActivateKeySeparation()

encoder.Encode(f, vals)
fmt.Println(vals) // output: map[y.a:[test] y.b:[true] x:[test]]

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Go Version Update
  • Dependency Update

Description

Related Tickets & Documents

Added/updated tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

Run verifications and test

  • make verify is passing
  • make test is passing

An Encoder can has the option to separate the names of nested
structs with a period when it constructs the values key during
encoding.
@gKits
Copy link
Author

gKits commented Nov 28, 2024

Make verify seems to fail due to some linter problems due to integer overflow conversions

@gKits gKits marked this pull request as ready for review November 28, 2024 12:32
@zak905
Copy link
Contributor

zak905 commented Nov 28, 2024

Looks good overall. If I may be a bit picky, I believe we could find a name which is semantically better for the option. I am thinking about something like: keepKeyOriginalPath or just keyOrignalPath

Also, I believe the Activate word is not necessary at the beginning of the setter function name. To keep in line with other options you can use the name directly with a boolean argument. the ZeroEmpty option is an example: https://github.com/gorilla/schema/blob/main/decoder.go#L46

@gKits
Copy link
Author

gKits commented Nov 29, 2024

@zak905 Thanks for the feedback. I have changed the name of the option to KeyOriginalPath definitely sounds better than before.

@gKits gKits requested a review from zak905 November 29, 2024 06:52
Copy link
Contributor

@zak905 zak905 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing the remark.

Waiting on @jaitaiwan feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Encoding of nested struct is missing named prefix
2 participants