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

Support embeddable structs #262

Merged
merged 10 commits into from Feb 14, 2022
Merged

Support embeddable structs #262

merged 10 commits into from Feb 14, 2022

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Feb 9, 2022

Hi! I've added support for embedded structs as discussed in this issue.

Both pointers and plain fields are supported.

type Person struct {
	ID   int
	Name string
}

type Employee struct {
	Person
	Salary int
}

By default embedded field names are added without prefixes. Name prefixes can be set like for other fields:

type Employee struct {
	Person   `db:"person_"`
	Salary int
}

If two fields have the same name, REL panics during construction:

What I've changed

  • documentData indices now store full field paths as []int
  • documentDatas are merged for embedded fields during creation
  • Embedded pointers are initialized during document creation

Tests

I've added tests for all main cases

association.go Outdated Show resolved Hide resolved
document.go Outdated Show resolved Hide resolved
@dranikpg dranikpg mentioned this pull request Feb 9, 2022
3 tasks
document_test.go Outdated Show resolved Hide resolved
@Fs02
Copy link
Member

Fs02 commented Feb 10, 2022

Thank you, this looking great 👍

A few things to improve, can you add:

  • Test case for getting value/primary key/primary field from embedded struct
  • Test case for setting value to embedded struct
  • How to handle embedded struct with duplicate fields

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #262 (415ef5d) into master (1bbbcfd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #262   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         3092      3135   +43     
=========================================
+ Hits          3092      3135   +43     
Impacted Files Coverage Δ
association.go 100.00% <100.00%> (ø)
collection.go 100.00% <100.00%> (ø)
document.go 100.00% <100.00%> (ø)
util.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bbbcfd...415ef5d. Read the comment docs.

@dranikpg
Copy link
Contributor Author

I've added support for name prefixes and updated the description.

How to handle embedded struct with duplicate fields

This is now checked. I've also added some more tests.


document.go is now three lines over 500 and codeclimate is complaining. I'd suggest moving pure reflect helper functions, like this to utils.

@Fs02
Copy link
Member

Fs02 commented Feb 11, 2022

document.go is now three lines over 500 and codeclimate is complaining. I'd suggest moving pure reflect helper functions, like this to utils.

Agreed 👍

@dranikpg
Copy link
Contributor Author

I've moved embedded pointer initialization to document creation. This is a more "eager" approach, but traversing reflect Values and checking nils for field reads is somewhat costly

association.go Outdated Show resolved Hide resolved
document.go Outdated Show resolved Hide resolved
@Fs02
Copy link
Member

Fs02 commented Feb 12, 2022

I've moved embedded pointer initialization to document creation

hmm, not sure about the correctness of this behavior.
for example if we query/preload a record, and it's not found, then we shouldn't modify the struct

@dranikpg
Copy link
Contributor Author

dranikpg commented Feb 12, 2022

I've moved embedded pointer initialization to document creation

hmm, not sure about the correctness of this behavior. for example if we query/preload a record, and it's not found, then we shouldn't modify the struct

So all field reads/writes should be wrapped with pointer checks? Not a problem, I just thought it's somewhat costly.
I guess reads should init pointers, or else functions like PrimaryValue() could fail to return a value 😶

We could Value() make only check for pointers without intializing them. But this would be inconvenient as there is no other way to initialize it other than setting one of its nested values

@Fs02
Copy link
Member

Fs02 commented Feb 12, 2022

Not a problem, I just thought it's somewhat costly

yes right, but it's for the correctness, unless there's better way to implement it 🤔

@Fs02
Copy link
Member

Fs02 commented Feb 13, 2022

So all field reads/writes should be wrapped with pointer checks? Not a problem, I just thought it's somewhat costly.
I guess reads should init pointers, or else functions like PrimaryValue() could fail to return a value 😶

after giving it some thought and re-reading existing implementation, I think reads shouldn't initialize pointer
it should just returns nil if the embedded struct in path is nil

document.go Outdated Show resolved Hide resolved
association.go Outdated Show resolved Hide resolved
association.go Outdated Show resolved Hide resolved
document.go Outdated Show resolved Hide resolved
document.go Outdated Show resolved Hide resolved
collection.go Outdated Show resolved Hide resolved
Copy link
Member

@Fs02 Fs02 left a comment

Choose a reason for hiding this comment

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

Thank you!!
just some more small comments, but overall already looking good 🎉

util.go Show resolved Hide resolved
document.go Outdated Show resolved Hide resolved
@Fs02 Fs02 merged commit 40789d1 into go-rel:master Feb 14, 2022
@Fs02
Copy link
Member

Fs02 commented Feb 14, 2022

Thanks for your contribution ❤️
if you have time, maybe you can also help updating the documentation?
https://github.com/go-rel/doc

@dranikpg
Copy link
Contributor Author

No problem, I'll have a look at it soon

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.

None yet

2 participants