Re: Beagle filter for Gentoo ebuilds

[Joe Shaw <joeshaw novell com>]

Hi,

On Sat, 2006-03-25 at 06:57 -0600, Pat Double wrote:
> Here's a better version, I'll attach this time. It handles bash line 
> continuation and I've changed from using "fixme:title" to "dc:title".

Great.

> Question, does the filter get instantiated once per beagle process, or for 
> every file? I am wondering if I should make the Regex fields static?

A new filter is instantiated each time.  So making them static is a good
idea.

The filter looks good, mostly I have stylistic comments.  Can you point
me to some ebuild files that I can use to test it?

>                 Regex METADATA_PATTERN = new Regex("\\s*(?<key>([A-Z_]+))\\s*=\\s*\"(?<value>(.*))\"\\s*");

Please put spaces after method names, but before parentheses.  Ie,
"Regex (foo)".  Also, METADATA_PATTERN should probably be named
metadata_pattern.  I'd suggest using "_pattern" for both EBUILD and
PACKAGE too.

See the HACKING file for the coding style standards.

>                 public FilterEbuild ()
>                 {
>                         AddSupportedFlavor (FilterFlavor.NewFromExtension (".ebuild"));
>                         SetVersion(2);

This SetVersion() isn't necessary (when it's checked in upstream).

>                         
>                         String version = match.Groups["version"].ToString();
>                         if (version.Length > 0) {
>                                 AddProperty (Beagle.Property.New ("fixme:version", version));
>                         }

I think that Beagle.Property.NewKeyword() is better here.  The main
difference is that New() gets stemmed, broken up on punctuation, etc.

>                                 if (str.Length > 0) {

Instead of adding a level of indentation for all the following code, I
suggest doing:

        if (str.Length == 0)
                continue;

>                                                         if (key.Equals("DESCRIPTION")) {
>                                                                 AddProperty (Beagle.Property.New ("dc:description", value));
>                                                         }
>                                                         else if (key.Equals("LICENSE")) {
>                                                                 AddProperty (Beagle.Property.New ("dc:rights", value));
>                                                         }

Generally speaking, we don't put single lines inside "if" statements in
brackets, but that's not a big deal.  More importantly, we put "else"
and "else if" on the same line as the brackets.  Ie:

        if (foo) {
                ;
        } else if (bar) {
                ;
        } else {
                ;
        }

>                                                         foreach (Match theMatch in matches) {

Please use "the_match" rather than "theMatch".

Thanks,
Joe