Add extends functionality to allow embedded fields #269
Conversation
Signed-off-by: Andrew Thornton <[email protected]>
|
Thanks for the PR! I feel like we're abusing the struct tags 😂 Any reason why the same purpose can't be achieved by: type testExtend struct {
BaseStruct `ini:"extended"`
Extend bool
}Which indicates the section name directly? |
|
Let's say we have the following sections: [foo]
Base=foobase
Extended=fooextended
[Base]
Base=barbase
[fubar]
Base=fubarbase
Extended=fubarextendedand we want to map both |
@zeripath Hmm... I'm kinda confused. Could you give more concrete example input and output based on this example INI? |
|
We have the following: [cron.check_repo_stats]
Enabled=true
RunAtStart=true
Schedule=@every 24 hours
[cron.archive_cleanup]
Enabled=true
RunAtStart=true
Schedule=@every 24h
OlderThan=24h
[cron.deleted_branches_cleanup]
Enabled=true
RunAtStart=true
Schedule=@every 24h
OlderThan=24h
[cron.sync_external_users]
Enabled=true
RunAtStart=false
Schedule=@every 24h
UpdateExisting=trueNow it would be nice if there was a clean struct family: // BaseConfig represents the basic config for a Cron task
type BaseConfig struct {
Enabled bool
RunAtStart bool
Schedule string
NoSuccessNotice bool
}
// OlderThanConfig represents a cron task with OlderThan setting
type OlderThanConfig struct {
BaseConfig
OlderThan time.Duration
}
// UpdateExistingConfig represents a cron task with UpdateExisting setting
type UpdateExistingConfig struct {
BaseConfig
UpdateExisting bool
}I have code like: RegisterTask("check_repo_stats", &BaseConfig{
Enabled: true,
RunAtStart: true,
Schedule: "@every 24h",
}, func(ctx context.Context, _ *models.User, _ Config) error {
return models.CheckRepoStats(ctx)
})where if err := Cfg.Section("cron." + name).MapTo(config); err != nil {
return config, err
}I therefore can't just use |
|
@zeripath Thanks for the example! I agree with adding this functionality, the concerns I have are:
Here is my alternative proposalSince mapping/reflecting of a section only happens with a In this package, we add an interface: // SectionExtender indicates the struct extends to the same section when mapping and reflecting.
type SectionExtender interface {
INIExtendsSection()
}Then modifying your example: // BaseConfig represents the basic config for a Cron task
type BaseConfig struct {
ini.SectionExtender
Enabled bool
RunAtStart bool
Schedule string
NoSuccessNotice bool
}
// OlderThanConfig represents a cron task with OlderThan setting
type OlderThanConfig struct {
BaseConfig
OlderThan time.Duration
}
// UpdateExistingConfig represents a cron task with UpdateExisting setting
type UpdateExistingConfig struct {
BaseConfig
UpdateExisting bool
}NOTE: This might have slight changes during implementation. WDYT? |
|
In terms of how you'd handle the I've had a few thoughts about the Happy to code that up and change this PR to match that - patch is in the Patch details section of this comment. I don't think labelling the base struct with an interface is a good idea - the struct doing the embedding should have control over how it's embedded structs are mapped. You may not always be able to or want to alter the base struct. It's really the wrong place to do this kind of thing. PatchFrom 61851be8890d822481d6b38afef89a6924c9b148 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <[email protected]>
Date: Sat, 3 Oct 2020 11:11:02 +0100
Subject: [PATCH] allow extends with name
Signed-off-by: Andrew Thornton <[email protected]>
---
struct.go | 33 +++++++++++++++++----------------
struct_test.go | 32 ++++++++++++++++----------------
2 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/struct.go b/struct.go
index 5bf79b1..fa30318 100644
--- a/struct.go
+++ b/struct.go
@@ -266,24 +266,18 @@ func setWithProperType(t reflect.Type, key *Key, field reflect.Value, delim stri
func parseTagOptions(tag string) (rawName string, omitEmpty bool, allowShadow bool, allowNonUnique bool, extends bool) {
opts := strings.SplitN(tag, ",", 5)
rawName = opts[0]
- if len(opts) > 1 {
- omitEmpty = opts[1] == "omitempty"
- }
- if len(opts) > 2 {
- allowShadow = opts[2] == "allowshadow"
- }
- if len(opts) > 3 {
- allowNonUnique = opts[3] == "nonunique"
- }
- if len(opts) > 4 {
- extends = opts[4] == "extends"
+ for _, opt := range opts[1:] {
+ omitEmpty = omitEmpty || (opt == "omitempty")
+ allowShadow = allowShadow || (opt == "allowshadow")
+ allowNonUnique = allowNonUnique || (opt == "nonunique")
+ extends = extends || (opt == "extends")
}
return rawName, omitEmpty, allowShadow, allowNonUnique, extends
}
// mapToField maps the given value to the matching field of the given section.
// The sectionIndex is the index (if non unique sections are enabled) to which the value should be added.
-func (s *Section) mapToField(val reflect.Value, isStrict bool, sectionIndex int) error {
+func (s *Section) mapToField(val reflect.Value, isStrict bool, sectionIndex int, sectionName string) error {
if val.Kind() == reflect.Ptr {
val = val.Elem()
}
@@ -315,7 +309,14 @@ func (s *Section) mapToField(val reflect.Value, isStrict bool, sectionIndex int)
if isStructPtr && field.IsNil() {
field.Set(reflect.New(tpField.Type.Elem()))
}
- if err := s.mapToField(field, isStrict, sectionIndex); err != nil {
+ fieldSection := s
+ if rawName != "" {
+ sectionName = s.name + s.f.options.ChildSectionDelimiter + rawName
+ if secs, err := s.f.SectionsByName(s.name + s.f.options.ChildSectionDelimiter + rawName); err == nil && len(secs) > 0 && sectionIndex < len(secs) {
+ fieldSection = secs[sectionIndex]
+ }
+ }
+ if err := fieldSection.mapToField(field, isStrict, sectionIndex, sectionName); err != nil {
return fmt.Errorf("map to field %q: %v", fieldName, err)
}
} else if isAnonymousPtr || isStruct || isStructPtr {
@@ -328,7 +329,7 @@ func (s *Section) mapToField(val reflect.Value, isStrict bool, sectionIndex int)
if isStructPtr && field.IsNil() {
field.Set(reflect.New(tpField.Type.Elem()))
}
- if err = secs[sectionIndex].mapToField(field, isStrict, sectionIndex); err != nil {
+ if err = secs[sectionIndex].mapToField(field, isStrict, sectionIndex, fieldName); err != nil {
return fmt.Errorf("map to field %q: %v", fieldName, err)
}
continue
@@ -367,7 +368,7 @@ func (s *Section) mapToSlice(secName string, val reflect.Value, isStrict bool) (
typ := val.Type().Elem()
for i, sec := range secs {
elem := reflect.New(typ)
- if err = sec.mapToField(elem, isStrict, i); err != nil {
+ if err = sec.mapToField(elem, isStrict, i, sec.name); err != nil {
return reflect.Value{}, fmt.Errorf("map to field from section %q: %v", secName, err)
}
@@ -397,7 +398,7 @@ func (s *Section) mapTo(v interface{}, isStrict bool) error {
return nil
}
- return s.mapToField(val, isStrict, 0)
+ return s.mapToField(val, isStrict, 0, s.name)
}
// MapTo maps section to given struct.
diff --git a/struct_test.go b/struct_test.go
index 50dcc76..83c1182 100644
--- a/struct_test.go
+++ b/struct_test.go
@@ -58,8 +58,8 @@ type testStruct struct {
Unused int `ini:"-"`
Unsigned uint
Omitted bool `ini:"omitthis,omitempty"`
- Shadows []string `ini:",,allowshadow"`
- ShadowInts []int `ini:"Shadows,,allowshadow"`
+ Shadows []string `ini:",allowshadow"`
+ ShadowInts []int `ini:"Shadows,allowshadow"`
BoolPtr *bool
BoolPtrNil *bool
FloatPtr *float64
@@ -90,7 +90,7 @@ type testPeer struct {
type testNonUniqueSectionsStruct struct {
Interface testInterface
- Peer []testPeer `ini:",,,nonunique"`
+ Peer []testPeer `ini:",nonunique"`
}
type BaseStruct struct {
@@ -98,7 +98,7 @@ type BaseStruct struct {
}
type testExtend struct {
- BaseStruct `ini:",,,,extends"`
+ BaseStruct `ini:",extends"`
Extend bool
}
@@ -478,7 +478,7 @@ FieldInSection = 6
}
type File struct {
- Sections []Section `ini:"Section,,,nonunique"`
+ Sections []Section `ini:"Section,nonunique"`
}
f := new(File)
@@ -770,18 +770,18 @@ path = /tmp/gpm-profiles/test1.profile
AllowShadows: true,
})
type ShadowStruct struct {
- StringArray []string `ini:"sa,,allowshadow"`
+ StringArray []string `ini:"sa,allowshadow"`
EmptyStringArrat []string `ini:"empty,omitempty,allowshadow"`
- Allowshadow []string `ini:"allowshadow,,allowshadow"`
- Dates []time.Time `ini:",,allowshadow"`
- Places []string `ini:",,allowshadow"`
- Years []int `ini:",,allowshadow"`
- Numbers []int64 `ini:",,allowshadow"`
- Ages []uint `ini:",,allowshadow"`
- Populations []uint64 `ini:",,allowshadow"`
- Coordinates []float64 `ini:",,allowshadow"`
- Flags []bool `ini:",,allowshadow"`
- None []int `ini:",,allowshadow"`
+ Allowshadow []string `ini:"allowshadow,allowshadow"`
+ Dates []time.Time `ini:",allowshadow"`
+ Places []string `ini:",allowshadow"`
+ Years []int `ini:",allowshadow"`
+ Numbers []int64 `ini:",allowshadow"`
+ Ages []uint `ini:",allowshadow"`
+ Populations []uint64 `ini:",allowshadow"`
+ Coordinates []float64 `ini:",allowshadow"`
+ Flags []bool `ini:",allowshadow"`
+ None []int `ini:",allowshadow"`
}
shadow := &ShadowStruct{
--
2.25.1 |
Signed-off-by: Andrew Thornton <[email protected]>
+ for _, opt := range opts[1:] {
+ omitEmpty = omitEmpty || (opt == "omitempty")
+ allowShadow = allowShadow || (opt == "allowshadow")
+ allowNonUnique = allowNonUnique || (opt == "nonunique")
+ extends = extends || (opt == "extends")Brilliant! Thanks for the patch ❤️ Alright, agree with the approach implemented in this PR then.
As long as this is also addressed/implemented. |
|
patch applied. |
Co-authored-by: ᴜɴᴋɴᴡᴏɴ <[email protected]>
unknwon
added
status: reviewed
|
https://github.com/go-ini/ini/releases/tag/v1.62.0 has been tagged for this merge. |
|
Thanks! |
What problem should be fixed?
Allow embedded structs to inherit from the same ini section.
Have you added test cases to catch the problem?
Yes
Signed-off-by: Andrew Thornton [email protected]