On Sat, Jul 12, 2008 at 02:35:48PM +0200, Jakub Narebski wrote:
> On Sat, Jul 12, 2008, Petr Baudis wrote:
> > Or if you mean foreach use, it's trivial to
> >
> > foreach (grep { /^gitweb\./ } keys %$config)
> >
> > or provide a method of Git::Config that will return this list, but it
> > does not seem appropriate to have specific Git::Config instances for
> > this. (Besides, if the script also needs to access a single variable
> > from a different section, it will need to re-parse the config again.)
> >
> > So I think your approach would be good only if there are multiple
> > methods of Git::Config that would operate on the whole config and needed
> > a way to be restricted; is that the case?
>
> On the other hand if one uses (assuming now Git::Config object API)
>
> $conf = $repo->get_config('section');
>
> one could access configuration variable values _without_ prefixing
> them with subsection name, i.e.
>
> $conf->get('variable');
>
> and not
>
> $conf->get('section.variable');
>
>
> I'm not sure if it is much of an improvement.
I would propose not to go with this feature yet and add it only when
justified by accompanied cleanups of real code using this.
> We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and
> (in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can
> have $c->get(<VAR>), $c->get_bool(<VAR>) etc.
I think I prefer the terser approach of value/bool/int now since it is
shorter and seems to stay pretty unambiguous. Besides, this way we can
easily make mutators by just adding second optional argument to these
methods.
> The former plays better with
>
> $r->get_config()->bool('gitweb.blame');
>
> but this is nevertheless not recommended workflow; you can use
>
> $r->config_bool('gitweb.blame');
>
> and it would be faster (unless some Perl/OO trickery with singletons
> and the like).
>
> Recommended workflow (code) is
>
> $c = $r->get_config();
> ...
> $c->get_bool('gitweb.blame');
>
> or something like that.
I thought that the get_config() method would be reusing existing
Git::Config instances no-matter-what? (Otherwise, you can get into
rather tricky issues like one part of the code modifying the config and
the other not noticing the change, etc. And I can se no benefit. If you
want to reload the config, we might have method like reload(), but I
don't think that would be very useful except perhaps in some kind of
mod_perl'd gitweb scenario.)
> >>> Also, having accessors for special types lets you return undef when
> >>> the type really isn't defined, instead of e.g. true with current
> >>> config_val_bool, which is clearly bogus and requires complicated code
> >>> on the user side.
> >>
> >> I don't follow you. Didn't we agree on casting an error when variable
> >> is not of given type?
> >
> > Sorry, s/type really/variable really/. According to your original code,
> >
> > config_val_bool(undef)
> >
> > would return true, while the undef could be from both non-existent and
> > unassigned variable. (This 'unassigned variables' case is really
> > annoying to handle.)
>
> That is why you should check exists($config{<VAR>}) first.
And that was my point when I talked about complicated code on the user
side.
But we're arguing about nothing, really, except the exact amount of
coolness of the Git::Config approach. :-)
--
Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
