-
Notifications
You must be signed in to change notification settings - Fork 19
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 configparser options #17
Conversation
* update keyValue regexp * add check if section was found before parsing keyValue * update value slice index * add tests
* add Options to control Parser behavior * add NewWithOptions * add ParseReaderWithOptions * add ParseWithOptions moved main parser logic into configparser.ParseReader
* interpolation * commentPrefixes * inlineCommentPrefixes * defaultSection * delimeters * converters * strict TODO: investigate `emptyLines` realisation
I do want to land these changes, I just need to find some time to read over them. |
I still do some things here and there |
options.go
Outdated
strict bool | ||
|
||
// TODO: under investigation, have no effect now. | ||
emptyLines bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bigkevmcd I was thinking recently about this option, and actually I think it can be implemented, but in this case we will need to change the basics of the file parser. Since it's related to #14 issue, the process of parsing the multiline values should be added first. In this case, I would like to leave it as it is right now and come back to it after those changes will be reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make time over the weekend to review this and move it forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added multiline value support and empty lines flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, some small changes, but otherwise, I'm happy enough with this.
configparser.go
Outdated
curSect = newSection(section) | ||
p.config[section] = curSect | ||
} else if p.opt.strict { | ||
return fmt.Errorf("section %q error: %w", section, ErrAlreadyExist) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure wrapping a sentinel error is the right way to do this.
I'd be inclined to define an AlreadyExistsError
type, which can contain the section with .Error() string
which would return this message.
Take a look at the stdlib PathError for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I also use it here to report duplicate option. As I see PathError
still have error field and Unwrap
method and Op
. Is it worth it to make the whole new structure for the error which actually can be triggered twice? Imho wrapped error is more informative, but at the same time can be easily unwrapped in the user code to assert it with errors.Is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed wrapped error and let it be just text description, without any specific type
options.go
Outdated
type Prefixes []string | ||
|
||
// AtFirst checks if str starts with any of the prefixes. | ||
func (pr Prefixes) AtFirst(str string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just MatchesPrefix
for this? or even HasPrefix
?
AtFirst
doesn't really say what this does?
func (pr Prefixes) Split(str string) string { | ||
for _, p := range pr { | ||
if strings.Contains(str, p) { | ||
return strings.Split(str, p)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that these are prefixes, should this use HasPrefix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used for inline comments e.g. option = value # this is a comment
. Thus, here it checks if given string contains any of the Prefixes
@@ -15,6 +15,10 @@ func New(dicts ...Dict) *ChainMap { | |||
return chainMap | |||
} | |||
|
|||
func (c *ChainMap) Add(dicts ...Dict) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exported and should have a doc comment?
@bigkevmcd should we add some info on the options usage in readme file? Like the same text in the MR description? |
@bigkevmcd any updates on this one? |
@emar-kar Apologies, I'll get to it this week. |
if section name equals default section name in options, then return default items instead of error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emar-kar Thanks for this, looks good.
This PR adds options which are available in python
ConfigParser
constructor.List of added options:
Prefixes
it will be passed during parsing.Prefixes
and if so, splits the string by the prefix and returns the 0 index of the slice.Prefixes
and if so, counts it as a part of the current value.true
, parser will return error for duplicates of sections or options in one source.true
allows multiline values to include empty lines as their part. Otherwise the value will be parsed until an empty line or the line which does not start with one of the allowed multiline prefixes.chainmap.ChainMap
instance.ConvertFunc
can modify requested value if needed e.g.,Those functions triggered inside
ConfigParser.Get*
methods if presented and wraps the return value.Converter
is amap
type, which supports int (forint64
), string, bool, float (forfloat64
) keys.Default options, which are always preset: