search

ohler55 / ojg

Optimized JSON for Go
MIT License
857 stars 49 forks source link

jp.reflectGetChild function enhanced #134

Closed thiagodpf closed 1 year ago

thiagodpf commented 1 year ago

Related to #133

I took as a basis the original function that was being used before: rd.FieldByNameFunc() (line 1590)

but as the original function did not provide the type reflect.StructField, only the field name, the adjustments I made were just to support this feature.

I ask you to check (diff) the PR code against the original function to see what I changed: reflect.FieldByNameFunc(match func(string) bool) https://cs.opensource.google/go/go/+/master:src/reflect/type.go;l=1025?q=type.go&ss=go%2Fgo

ohler55 commented 1 year ago

It's interesting that the search for a name considers the possibility of recursion in the embedded structs but is that even possible in go? You might be doing more work there than is necessary. Can you provide a test case that exercises all that code?

thiagodpf commented 1 year ago

It's interesting that the search for a name considers the possibility of recursion in the embedded structs but is that even possible in go? You might be doing more work there than is necessary. Can you provide a test case that exercises all that code?

It is fascinating how it is possible to compose structs in Golang. Despite being edge cases, they are doable! I've included two more test cases, one that explores the scenario of attributes with duplicate names and another with a cyclic graph.

I think you can explore the possibilities even more... See that I didn't create the algorithm, I just derived it from the original you were using. See the similarity at: https://cs.opensource.google/go/go/+/master:src/reflect/type.go;l=1025?q=type.go&ss=go%2Fgo

It's a pity that the original function doesn't give us the option of providing a "lambda" that takes the reflect.StructField as an argument instead of the string containing the field name. Sad but maybe one day they will make this available to us (then this workaround can be removed).

ohler55 commented 1 year ago

I'll look over the code again tonight and verify the coverage before merging.

ohler55 commented 1 year ago

Can you address the linter warning? I know the go code itself often fails the linter and deviates from their stated conventions so a direct copy isn't always possible.

thiagodpf commented 1 year ago

Sorry, didn't notice the lint. I think I fixed it now.

ohler55 commented 1 year ago

I'll make a changelog entry and release tonight. Thanks for the contribution.