[pacman-dev] [PATCH 1/1] scripts/library: add config_parser.sh

[lolilolicon]

On Fri, Sep 30, 2011 at 8:12 PM, Dave Reisner <d at falconindy.com> wrote:
> On Fri, Sep 30, 2011 at 07:37:48PM +0800, lolilolicon wrote:
>> config_parser.sh provides functions to support parsing simple config
>> files, pacman.conf in particular.  Our scripts should never use a
>> simple eval to get config values such as DBPath.
>>
>> conf_key_val() is modified from pacman-key's get_from() function. Please
>> note conf_key_val() and conf_key_set() both read config file from stdin,
>> instead of $1.
>
> I don't see the advantage of doing it this way. If/when we add a
> function like conf_keyval_set() (which actually updates a file) we'll
> have to ignore this convention and pass the filename, for obvious
> reasons.
>

Well I called it config_parser. I meant for it to *read* files only.
For readers reading from stdin is more natural. I see both functions as a
variant of `read'... We can pipe around more easily, without the need of
process substitution.

And we don't have a convention yet. After all, getters and setters are two
differnt things, I don't think we have to unify the syntax.

> At the risk of painting my shed green, I dislike your naming convention,
> particularly conf_key_set, which doesn't set anything at all. I would
> have thought that something such as the following would be appropriate:
>
> # accessors
> conf_key_isset() # determines existance of a key w/o an associated value.
> conf_keyval_get() # gets the value of a key.
>
> # mutators
> conf_key_set() # sets a key without a value, as long as it doesn't exist.
> conf_keyval_set() # sets a key with a value, as long as it doesn't exist.
>
> And of course, accessors return 0/1 to indicate existance, mutators
> return 0/1 to indicate write success/failure.
>
> d
>

Again I didn't intend to implement mutators. But if I did, I would name them:

# accessors
conf_key_val  # print the value of key
conf_key_set  # Is key set?

# mutators
conf_set_key    # set a key to either "on" or =value [1]
conf_unset_key  # simply comment the line(s) setting key

[1] conf_set_key would add the =value part only if value is specified:

conf_set_key conf key        # set key on
conf_set_key conf key value  # set key to value

I think it would be pretty clear _by comparison_ that:

 - The 'set' in conf_key_set is a past participle. => accessor
 - The 'set' in conf_set_key is a verb. => mutator

It would be hard to mix those two up if we implemented them both.

At last, I just wanted to make myself clear. I'm open to name changes :)

>> Signed-off-by: lolilolicon <lolilolicon at gmail.com>
>> ---
>>  scripts/library/README           |    3 ++
>>  scripts/library/config_parser.sh |   56 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 59 insertions(+), 0 deletions(-)
>>  create mode 100644 scripts/library/config_parser.sh
>>
>> diff --git a/scripts/library/README b/scripts/library/README
>> index 1e9c962..4fb70dd 100644
>> --- a/scripts/library/README
>> +++ b/scripts/library/README
>> @@ -1,6 +1,9 @@
>>  This folder contains code snippets that can be reused by multiple
>>  scripts.  A brief description of each file follows.
>>
>> +config_parser.sh:
>> +Parses simple config files such as pacman.conf.
>> +
>>  output_format.sh:
>>  Provides basic output formatting functions with levels 'msg', 'msg2',
>>  'warning' and 'error'.  The 'msg' amd 'msg2' functions print to stdout
>> diff --git a/scripts/library/config_parser.sh b/scripts/library/config_parser.sh
>> new file mode 100644
>> index 0000000..1d125eb
>> --- /dev/null
>> +++ b/scripts/library/config_parser.sh
>> @@ -0,0 +1,56 @@
>> +shopt -s extglob
>> +
>> +# Parse value of a non-repeating key from a config file.
>> +#
>> +# stdin: config file
>> +# $1: key - shall not contain '=', or start/end with whitespaces
>> +# $2: default value if not defined in config file [optional]
>> +# return:
>> +#   0 - print value of key
>> +#   1 - print nothing
>> +conf_key_val() {
>> +     local key val
>> +     while IFS='=' read -r key val; do
>> +             # strip whitespaces
>> +             key=${key##*([[:space:]])}
>> +             key=${key%%*([[:space:]])}
>> +             val=${val##*([[:space:]])}
>> +             val=${val%%*([[:space:]])}
>
> read is the opportune thing to use here -- it's faster (which surprises
> me) and more concise.
>
>  read -r key <<< "$key"
>  read -r val <<< "$val"

Nice tip! I like it, and it also does away the need of extglob...