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

API of Config->set, get to replace str, int, bool #209

Closed
waterkip opened this issue Apr 19, 2019 · 7 comments
Closed

API of Config->set, get to replace str, int, bool #209

waterkip opened this issue Apr 19, 2019 · 7 comments

Comments

@waterkip
Copy link
Contributor

waterkip commented Apr 19, 2019

Hi, I wasn't really happy with str_add because it leaves much of the
powerfull api that the git command line util has (or the git-api itself,
pick any :)).

the git config manual/help states:

--bool, --int, --bool-or-int, --path, --expiry-date
Historical options for selecting a type specifier. Prefer
instead --type (see above).

I want thinking about using changing/replacing the interface of str
and str_add and implement a set and get method. set
could allow the following options:

# Just set one value: similar to `git config key bar'
$rv = Git::Raw::Config->set('key', 'bar'); # $rv yields
'bar';

# Implicit multi-var, similar to `git config --add key foo && git config --add key bar'
$rv = Git::Raw::Config->set('key', [qw(foo bar)]); # $rv yields [ 'foo', 'bar' ]

# Explicit multi-var
$rv = Git::Raw::Config->set('key', 'foo', { add => 1 }); # $rv yields [ 'foo']
$rv = Git::Raw::Config->set('key', $bar, { add => 1 }); # explicit multi var setting, $rv yields [ 'foo', bar' ]

# Change all values: `git config --replace-all key foo'
$rv = Git::Raw::Config->set('key', 'value', { replace_all=> 1 }); # $rv yields 'value'

# `git config --replace-regex'
$rv = Git::Raw::Config->set('key', 'value', { replace_all => 1, regexp=> qr/^foo/ }); # $rv yields 'value' if just one
entry is found and [ 'bar', 'value' ] if the key was multi
var with 'bar' and 'foo' as its values.

# This will replace(/deprecate?)
set('key', 'value', { type => 'str' }); # replaces str()
set('key', $value, { type => int }); # replaces int()
set('key, $value, { type => bool}); # replaces bool()

There is also --unset, which could also be implemented by
set, ala

set('key', undef); # croaks?

set('key', undef, { unset => 1}); # removes the value
set('key', undef, { unset => 1, regexp => qr//, }); # removes the value if matches the regexp

set('key', undef, { unset_all => 1); # removes all the keys
set('key', undef, { unset_all => 1, regexp => qr//, }); # removes value from key if matches the regexp

I'm not certain what the return values should be? Similar to multi-var?

get would be similar in its api:

# git config --get
$rv = Git::Raw::Config->get('key'); # $rv yields 'value'

# git config --get-all
$rv = Git::Raw::Config->get('key', { get_all=> 1 }); # $rv yields [ 'value', ... ]

# git config --get --value-regexp
$rv = Git::Raw::Config->get('key', { regexp => qr// }) #  $rv yields 'value'

# git config --get-regexp
$rv = Git::Raw::Config->get(qr/key/) # $rv yields { key => value }
$rv = Git::Raw::Config->get(qr/key/, { regexp => qr// }) # $rv yields { key => value }

Do you agree on this approach?

@jacquesg
Copy link
Owner

This seems reasonable, here are my thoughts:

  • Multi-var values should be passed directly on the stack instead of via a list reference.
  • The options controlling the behaviour of set should be the first argument after 'key'.
  • Add an unset method instead of passing undef to set. unset should return the value(s) that was set.
  • I would implement the existing str, int and bool in terms of set and unset.
  • I would define constants for the "type" i.e. type => 'str' would become Git::Raw::Config->STRING

@waterkip
Copy link
Contributor Author

I'm not really a fan of putting the options directly after the key. If you wouldn't use the options, you would get something like set('foo', {}, 'value') which doesn't look nice, or undef instead of {}.

@jacquesg
Copy link
Owner

The other way to do it is to make it the (optional) first parameter to set.

@waterkip
Copy link
Contributor Author

waterkip commented May 2, 2019

I am curious about why you want this, it has the same rather inconsistent API. The options are optional and aren't needed per se to set the value. So why they come first is something I do not understand.

@jacquesg
Copy link
Owner

jacquesg commented May 2, 2019

I want any key and values to be adjacent, so that the following works:

my @keyAndValue = ('key', 'value');
$config->set ({options1 => 1}, @keyAndValue);

If the optional options argument slices key and value the above won't work.

@jacquesg
Copy link
Owner

jacquesg commented May 2, 2019

I guess you could also make it work by putting the options last. It actually doesn't matter that much...

@jacquesg
Copy link
Owner

jacquesg commented May 2, 2019

Feel free to make it the last argument.

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

No branches or pull requests

2 participants