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.

Sign up for GitHub

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

Jump to bottom

Slice of structs support #10

Merged
merged 6 commits into from
Jan 30, 2019
Merged

Slice of structs support #10

merged 6 commits into from
Jan 30, 2019

Conversation

SpectrumQT
Copy link
Contributor

As for me, auto-iterating over slices of structs feels more like expected behaviour. Please review.

Original issue: #9

@SpectrumQT
Update defaults.go
bdf5cb5 
Added slice of struct support, tag is not needed
@SpectrumQT
Update defaults.go
757e8c2 
Added slice of struct support, tag is not needed
creasty
creasty requested changes Jan 28, 2019
View reviewed changes
Copy link
Owner

@creasty creasty left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
Please make sure it passes CI.

defaults.go Outdated
case reflect.Slice:
for j := 0; j < field.Len(); j++ {
sliceItem := field.Index(j)
if sliceItem.Kind() != reflect.Struct {
Copy link
Owner

Choose a reason for hiding this comment

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

Might want to call setField recursively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely. Updated commit.

@creasty
Copy link
Owner

creasty commented Jan 28, 2019

Oh, let me fix the CI. It seems to be outdated...

$ go get -u github.com/golang/lint/golint
# golang.org/x/tools/go/internal/gcimporter
../../../golang.org/x/tools/go/internal/gcimporter/bexport.go:212: obj.IsAlias undefined (type *types.TypeName has no field or method IsAlias)
The command "go get -u github.com/golang/lint/golint" failed and exited with 2 during .

https://travis-ci.org/creasty/defaults/builds/485234065?utm_source=github_status&utm_medium=notification

@creasty
Copy link
Owner

creasty commented Jan 28, 2019

Meanwhile, could you add test cases?

@SpectrumQT
Copy link
Contributor Author

SpectrumQT commented Jan 28, 2019 •
edited
Loading

Please make sure it passes CI.
If I get it right, defaults_test.go tests fails and I should edit it? Sorry, I'm new to this stuff. I'll try.

@creasty
Copy link
Owner

creasty commented Jan 28, 2019

If I get it right, defaults_test.go tests fails and I should edit it?

Not sure if it will break the existing test cases. But, either way, you'll need to edit it.

@creasty creasty mentioned this pull request Jan 28, 2019
Merged
@creasty
Copy link
Owner

creasty commented Jan 28, 2019

Oh, let me fix the CI. It seems to be outdated...

Fixed here #11

@codecov-io
Copy link

codecov-io commented Jan 28, 2019 •
edited
Loading

Codecov Report

Merging #10 into master will decrease coverage by 1.7%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage     100%   98.29%   -1.71%     
==========================================
  Files           2        2              
  Lines         111      117       +6     
==========================================
+ Hits          111      115       +4     
- Misses          0        1       +1     
- Partials        0        1       +1
Impacted Files Coverage Δ
defaults.go 98.24% <77.77%> (-1.76%) ⬇️

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 a7e48e2...8262bff. Read the comment docs.

@SpectrumQT
Copy link
Contributor Author

SpectrumQT commented Jan 28, 2019 •
edited
Loading

Fine, I've implemented new test for this feature and ensured passage of the old ones. Please take a look.

creasty
creasty approved these changes Jan 30, 2019
View reviewed changes
Copy link
Owner

@creasty creasty left a comment

Choose a reason for hiding this comment

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

Looking great!
Thanks 👍

@creasty creasty merged commit edf4f6a into creasty:master Jan 30, 2019
@creasty
Copy link
Owner

creasty commented Jan 30, 2019

Released: https://github.com/creasty/defaults/releases/tag/v1.3.0

@SpectrumQT
Copy link
Contributor Author

Was glad to contribute. Thanks for your little handy lib, my utility's config really loved it!

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

@creasty creasty creasty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SpectrumQT @creasty @codecov-io