ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Atherton <br...@callenish.com>
Subject RE: [WIP]Selectors
Date Mon, 04 Mar 2002 17:57:45 GMT
At 09:42 PM 3/4/2002 +1000, Adam Murdoch wrote:
>Some comments.  I think the selector idea is fantastic, so these comments
>are just implementation nit-picks.

All feedback very much appreciated.

>ISelector:
>
>* Can we do something about the name?  Almost none of the interfaces in Ant
>use the IBlah naming convention.  Can we change it to FileSelector, or
>something like that?

Sure, if you like. I was trying to find a name that told people this was an 
interface, and the "old school" way of doing that in Java is with a leading 
"I". But you are right, it isn't the way that the rest of Ant does things, 
so why not conform to the existing way of doing things, which is to ignore 
the fact that something is an interface in the name of it.

>* assignProject() isn't really necessary.  If a selector needs to get hold
>of the project, it can extend ProjectComponent, and the introspector will
>take care handing it a project.  No point setting up parallel infrastructure
>for doing this.

Ok, I wasn't aware there was another mechanism. I'm a little concerned, 
though, since you can't extend from multiple base classes. If you need the 
project in a SelectorContainer, what do you do?

I'd like to drop the method altogether, since I think it makes selectors 
far too aware of the environment they are running in. Ideally, they are 
encapsulated to just be classes that know how to pass or veto files without 
any knowledge of the context they are doing that in. Trouble is, I can 
imagine there being selectors that need to get at something elsewhere in 
the build, which is why I provided the method.

Perhaps the need is rare enough that your solution is better, and anyone 
who needs a selector container with the Project can just reimplement 
SelectorContainer.

>* Maybe add "throws BuildException" to isSelected().  How about we lose
>checkForError() as well, and get isSelected() to assert that things are
>good, throwing a BuildException if they're not.

I really tried to stay away from throwing BuildExceptions here for the 
reason listed above, it breaks encapsulation by making selectors aware they 
are running in the context of an ant build. Pragmatically, it probably 
doesn't make a difference (is anyone going to reuse selectors outside of 
ant?), it is just a theoretical concern. So perhaps tying selectors to ant 
is just fine, since it would simplify the interface as you say. What do you 
think?

>* Given the above, it might be worth making the interface completely
>stateless, and pass the base directory in to isSelected(), rather than using
>assignBasedir().

Yes, if we go to one method only that is definitely the right answer. 
Although...

One other feature I was thinking about adding is for the selector to keep 
track of the files it has selected and vetoed, along with a reset method 
for when it is being referenced in another fileset. That would give 
everything you get from FileScanner and then some. But it would also break 
the statelessness.

>SelectorContainer:
>
>* Should assignProject() and assignBasedir() be calling through to the
>nested selectors?

And that's why it is a WIP. Of course they should. Thanks.

>* The createBlah() methods should probably be addBlah() methods instead.

Ok. Is there a reason to prefer one over the other? I've always used create 
because it gives more flexibility.

>NotSelector:
>
>* isSelected() probably should assert that there is exactly one nested
>selector.  Otherwise you've got a nor selector, not a not selector.

I asked this question earlier and got no response, so I just did what 
seemed most intuitive to me.

Hmm. I could always do both, but I'm not sure it would be clear to people 
what the difference between <not> and <nor> is. And I was really trying to 
stay away from <nand> since unless you work with logic gates, that is not 
in common use.

Is the issue that it isn't clear to people with multiple selectors in a 
<not> what the outcome would be, and that at times their intuition would be 
wrong?

>FilenameSelector:
>
>* Do we really need 'negate'?  How about we take it out and wait to see if
>it's useful.

I went with Stefan's suggestion of putting it only where it is common. 
Since people use <exclude> an awful lot, I thought this a likely candidate. 
Perhaps not.

>* The guts of this selector look suspiciously like the guts of
>DirectoryScanner.  Can you refactor the matching code into a ScannerUtil
>class that both FileNameSelector and DirectoryScanner share? (it's mostly
>all static, shouldn't be a drama).

I would love to, but in a slightly different way.

I think it is silly for DirectoryScanner to be deciding which files are 
included in a fileset. It should just be a driver, reading the file entries 
and passing them into the fileset/patterset/selector to make that decision. 
Selectors moves some of that responsibility into the selector classes, 
where it belongs, but the includes and excludes are still handled in 
DirectoryScanner.

That could be changed, though. If creating an include or an exclude didn't 
just add the PatternSet.NameEntry, but also created a



>IExtendSelector:
>
>* Hmmm.  setAttrib9()?  Can we use the same pattern for extensibility that
>the file filter code uses?  Whether it's a good or bad pattern, we need
>consistency.  I suspect that there's a good candidate somewhere there for a
>base class gets used by the mapper, file filter, and selector extensibility
>beans, to take care of 90% of it.
>
>
>MajoritySelector:
>
>* Nice idea.
>
>
>PatternSet:
>
>I think you're overloading a little by adding the nested selectors to
>PatternSet.  Let's leave PatternSet as a set of patterns, and add a new
>selector container data-type (like AndSelector, say).  Get FileSet to extend
>that, rather than PatternSet.
>
>We probably still want PatternSet to *be* a selector, so rather than have
>PatternSet extend AndSelector, we should have it implement ISelector, as a
>specialisation of FileNameSelector.  FileSet will need to deal with
>PatternSet specially, to remain backwards compatible (since the PatternSets
>are aggregated before they are evaluated).
>
>
>Adam
>
> > -----Original Message-----
> > From: Bruce Atherton [mailto:bruce@callenish.com]
> > Sent: Monday, 4 March 2002 5:06 PM
> > To: ant-dev@jakarta.apache.org
> > Subject: [WIP]Selectors
> >
> >
> > Here is the Selectors as a work in progress, just so everyone can
> > see how it
> > is coming along. I still have some known problem areas, and much of this
> > hasn't been tested, but the earlier the feedback the better, right?
> >
> > I've included a test file that downloads ant 1.4.1 and does a few
> > selections
> > with it. This needs to be modified for the real testing framework
> > (which I
> > haven't paid any attention to so far, I'm afraid, but that I will have to
> > learn soon I guess) but it gives some reassurances that things
> > are working as
> > expected for at least simple situations.
> >
> > All changes so far pass the full testing framework "ant test" target, BTW.
> >
> > Anyway, the renaming is done, <and>, <or>, and <not> are
> > available, basically
> > <extendselect> works, in theory references should work (never
> > tested, so I'm
> > not sure), embedded <patternset>s in selector containers have
> > known problems,
> > I've simplified maintaining the static selectors by making a slight (but
> > fully backwards compatible) change to the inheritance of <patternset> and
> > <fileset>, and I've implemented a couple more selectors:
> > <majority>, which is
> > there to show people they can do more with selector containers than just
> > boolean; and <filenameselect>, which is also available as an
> > <extendselect>
> > class for testing purposes and will eventually play a role in fixing
> > <patternset>s that are within an explicit selector container.
> >
> > I've included the patch for the 3 changed files again, for
> > convenience. The
> > code changes are minimal so far, really, the biggest impact is to the
> > comments.
> >
> > All comments, criticisms, complaints, and any other c-words
> > welcome. Thanks.
> >
> >
> > Index: DirectoryScanner.java
> > ===================================================================
> > RCS file:
> > /home/cvspublic/jakarta-ant/src/main/org/apache/tools/ant/Director
> > yScanner.java,v
> > retrieving revision 1.22
> > diff -u -b -r1.22 DirectoryScanner.java
> > --- DirectoryScanner.java     18 Feb 2002 18:27:58 -0000      1.22
> > +++ DirectoryScanner.java     4 Mar 2002 06:39:12 -0000
> > @@ -58,25 +58,31 @@
> >  import java.util.Vector;
> >  import java.util.StringTokenizer;
> >
> > +import org.apache.tools.ant.types.selectors.SelectorScanner;
> > +import org.apache.tools.ant.types.selectors.ISelector;
> > +
> >  /**
> >   * Class for scanning a directory for files/directories which
> > match certain
> >   * criteria.
> >   * <p>
> > - * These criteria consist of a set of include and exclude patterns. With
> > these
> > - * patterns, you can select which files you want to have
> > included, and which
> > - * files you want to have excluded.
> > + * These criteria consist of a set of selectors which have been
> > specified.
> > + * With these selectors you can select which files you want to
> > have included.
> > + * Files which are not selected are excluded.
> >   * <p>
> >   * The idea is simple. A given directory is recursively scanned
> > for all files
> > - * and directories. Each file/directory is matched against a set
> > of include
> > + * and directories. Each file/directory is matched against a set of
> > selectors,
> > + * including special support for matching agains filenames with
> > include and
> >   * and exclude patterns. Only files/directories which match at least one
> > - * pattern of the include pattern list, and don't match any
> > pattern of the
> > - * exclude pattern list will be placed in the list of files/directories
> > found.
> > + * pattern of the include pattern list or other file selector, and don't
> > match
> > + * any pattern of the exclude pattern list or fail to match against a
> > required
> > + * selector will be placed in the list of files/directories found.
> >   * <p>
> >   * When no list of include patterns is supplied, "**" will be used, which
> >   * means that everything will be matched. When no list of
> > exclude patterns is
> > - * supplied, an empty list is used, such that nothing will be excluded.
> > + * supplied, an empty list is used, such that nothing will be
> > excluded. When
> > + * no selectors are supplied, none are applied.
> >   * <p>
> > - * The pattern matching is done as follows:
> > + * The filename pattern matching is done as follows:
> >   * The name to be matched is split up in path segments. A path
> > segment is the
> >   * name of a directory or file, which is bounded by
> >   * <code>File.separator</code> ('/' under UNIX, '\' under Windows).
> > @@ -140,7 +146,7 @@
> >   * <a href="mailto:ajkuiper@wxs.nl">ajkuiper@wxs.nl</a>
> >   * @author <a href="mailto:umagesh@rediffmail.com">Magesh Umasankar</a>
> >   */
> > -public class DirectoryScanner implements FileScanner {
> > +public class DirectoryScanner implements FileScanner, SelectorScanner {
> >
> >      /**
> >       * Patterns which should be excluded by default.
> > @@ -170,10 +176,15 @@
> >      /** The patterns for the files to be excluded. */
> >      protected String[] excludes;
> >
> > -    /** The files which matched at least one include and no excludes. */
> > +    /** Selectors that will filter which files are in our
> > candidate list. */
> > +    protected ISelector[] selectors = null;
> > +
> > +    /** The files which matched at least one include and no excludes
> > +     *  and were selected.
> > +     */
> >      protected Vector filesIncluded;
> >
> > -    /** The files which did not match any includes. */
> > +    /** The files which did not match any includes or selectors. */
> >      protected Vector filesNotIncluded;
> >
> >      /**
> > @@ -182,7 +193,9 @@
> >       */
> >      protected Vector filesExcluded;
> >
> > -    /** The directories which matched at least one include and
> > no excludes.
> > */
> > +    /** The directories which matched at least one include and
> > no excludes
> > +     *  and were selected.
> > +     */
> >      protected Vector dirsIncluded;
> >
> >      /** The directories which were found and did not match any
> > includes. */
> > @@ -194,6 +207,16 @@
> >       */
> >      protected Vector dirsExcluded;
> >
> > +    /** The files which matched at least one include and no excludes and
> > +     *  which a selector discarded.
> > +     */
> > +    protected Vector filesDeselected;
> > +
> > +    /** The directories which matched at least one include and
> > no excludes
> > +     *  but which a selector discarded.
> > +     */
> > +    protected Vector dirsDeselected;
> > +
> >      /** Whether or not our results were built by a slow scan. */
> >      protected boolean haveSlowResults = false;
> >
> > @@ -711,6 +734,17 @@
> >          }
> >      }
> >
> > +
> > +    /**
> > +     * Sets the selectors that will select the filelist.
> > +     *
> > +     * @param selectors specifies the selectors to be invoked on a scan
> > +     */
> > +    public void setSelectors(ISelector[] selectors) {
> > +        this.selectors = selectors;
> > +    }
> > +
> > +
> >      /**
> >       * Sets the list of exclude patterns to use. All '/' and '\'
> > characters
> >       * are replaced by <code>File.separatorChar</code>, so the separator
> > used
> > @@ -752,7 +786,8 @@
> >
> >      /**
> >       * Scans the base directory for files which match at least
> > one include
> > -     * pattern and don't match any exclude patterns.
> > +     * pattern and don't match any exclude patterns. If there
> > are selectors,
> > +     * then the files must pass muster there, as well.
> >       *
> >       * @exception IllegalStateException if the base directory was set
> >       *            incorrectly (i.e. if it is <code>null</code>, doesn't
> > exist,
> > @@ -783,14 +818,20 @@
> >          filesIncluded    = new Vector();
> >          filesNotIncluded = new Vector();
> >          filesExcluded    = new Vector();
> > +        filesDeselected      = new Vector();
> >          dirsIncluded     = new Vector();
> >          dirsNotIncluded  = new Vector();
> >          dirsExcluded     = new Vector();
> > +        dirsDeselected       = new Vector();
> >
> >          if (isIncluded("")) {
> >              if (!isExcluded("")) {
> > +                if (isSelected("",basedir)) {
> >                  dirsIncluded.addElement("");
> >              } else {
> > +                    dirsDeselected.addElement("");
> > +                }
> > +            } else {
> >                  dirsExcluded.addElement("");
> >              }
> >          } else {
> > @@ -838,8 +879,8 @@
> >      /**
> >       * Scans the given directory for files and directories.
> > Found files and
> >       * directories are placed in their respective collections,
> > based on the
> > -     * matching of includes and excludes. When a directory is
> > found, it is
> > -     * scanned recursively.
> > +     * matching of includes, excludes, and selectors. When a directory is
> > +     * found, it is scanned recursively.
> >       *
> >       * @param dir   The directory to scan. Must not be <code>null</code>.
> >       * @param vpath The path relative to the base directory (needed to
> > @@ -876,12 +917,21 @@
> >              if (file.isDirectory()) {
> >                  if (isIncluded(name)) {
> >                      if (!isExcluded(name)) {
> > +                        if (isSelected(name,file)) {
> >                          dirsIncluded.addElement(name);
> >                          if (fast) {
> >                              scandir(file, name+File.separator, fast);
> >                          }
> >                      } else {
> >                          everythingIncluded = false;
> > +                            dirsDeselected.addElement(name);
> > +                            if (fast && couldHoldIncluded(name)) {
> > +                                scandir(file, name+File.separator, fast);
> > +                            }
> > +                        }
> > +
> > +                    } else {
> > +                        everythingIncluded = false;
> >                          dirsExcluded.addElement(name);
> >                          if (fast && couldHoldIncluded(name)) {
> >                              scandir(file, name+File.separator, fast);
> > @@ -900,9 +950,14 @@
> >              } else if (file.isFile()) {
> >                  if (isIncluded(name)) {
> >                      if (!isExcluded(name)) {
> > +                        if (isSelected(name,file)) {
> >                          filesIncluded.addElement(name);
> >                      } else {
> >                          everythingIncluded = false;
> > +                            filesDeselected.addElement(name);
> > +                        }
> > +                    } else {
> > +                        everythingIncluded = false;
> >                          filesExcluded.addElement(name);
> >                      }
> >                  } else {
> > @@ -965,6 +1020,24 @@
> >      }
> >
> >      /**
> > +     * Tests whether a name should be selected.
> > +     *
> > +     * @param name the name to check for selecting
> > +     * @return <code>false</code> when at least one selector
> > says that the
> > +     *         name should not be selected, <code>true</code> otherwise.
> > +     */
> > +    protected boolean isSelected(String name, File file) {
> > +        if (selectors != null) {
> > +            for (int i = 0; i < selectors.length; i++) {
> > +                if ((selectors[i].isSelected(name, file)) == false) {
> > +                    return false;
> > +                }
> > +            }
> > +        }
> > +        return true;
> > +    }
> > +
> > +    /**
> >       * Returns the names of the files which matched at least one of the
> >       * include patterns and none of the exclude patterns.
> >       * The names are relative to the base directory.
> > @@ -1023,6 +1096,25 @@
> >      }
> >
> >      /**
> > +     * Returns the names of the files which were selected. The names
> > +     * are relative to the base directory. This involves performing
> > +     * a slow scan if one has not already been completed.
> > +     *
> > +     * @return the names of the files which were selected.
> > +     *
> > +     * @see #slowScan
> > +     */
> > +    public String[] getDeselectedFiles() {
> > +        slowScan();
> > +        int count = filesDeselected.size();
> > +        String[] files = new String[count];
> > +        for (int i = 0; i < count; i++) {
> > +            files[i] = (String)filesDeselected.elementAt(i);
> > +        }
> > +        return files;
> > +    }
> > +
> > +    /**
> >       * Returns the names of the directories which matched at
> > least one of
> > the
> >       * include patterns and none of the exclude patterns.
> >       * The names are relative to the base directory.
> > @@ -1076,6 +1168,25 @@
> >          String[] directories = new String[count];
> >          for (int i = 0; i < count; i++) {
> >              directories[i] = (String)dirsExcluded.elementAt(i);
> > +        }
> > +        return directories;
> > +    }
> > +
> > +    /**
> > +     * Returns the names of the directories which were selected.
> > The names
> > +     * are relative to the base directory. This involves performing a
> > +     * slow scan if one has not already been completed.
> > +     *
> > +     * @return the names of the directories which were selected.
> > +     *
> > +     * @see #slowScan
> > +     */
> > +    public String[] getDeselectedDirectories() {
> > +        slowScan();
> > +        int count = dirsDeselected.size();
> > +        String[] directories = new String[count];
> > +        for (int i = 0; i < count; i++) {
> > +            directories[i] = (String)dirsDeselected.elementAt(i);
> >          }
> >          return directories;
> >      }
> > Index: types/FileSet.java
> > ===================================================================
> > RCS file:
> > /home/cvspublic/jakarta-ant/src/main/org/apache/tools/ant/types/Fi
> > leSet.java,v
> > retrieving revision 1.22
> > diff -u -b -r1.22 FileSet.java
> > --- types/FileSet.java        26 Jan 2002 20:16:22 -0000      1.22
> > +++ types/FileSet.java        4 Mar 2002 06:39:13 -0000
> > @@ -58,6 +58,9 @@
> >  import org.apache.tools.ant.FileScanner;
> >  import org.apache.tools.ant.DirectoryScanner;
> >  import org.apache.tools.ant.Project;
> > +import org.apache.tools.ant.types.selectors.ISelector;
> > +import org.apache.tools.ant.types.selectors.SelectorScanner;
> > +import org.apache.tools.ant.types.selectors.AndSelector;
> >
> >  import java.io.File;
> >  import java.util.Stack;
> > @@ -74,7 +77,7 @@
> >   * @author <a href="mailto:stefan.bodewig@epost.de">Stefan Bodewig</a>
> >   * @author <a href="mailto:umagesh@rediffmail.com">Magesh Umasankar</a>
> >   */
> > -public class FileSet extends DataType implements Cloneable {
> > +public class FileSet extends PatternSet implements Cloneable {
> >
> >      private PatternSet defaultPatterns = new PatternSet();
> >      private Vector additionalPatterns = new Vector();
> > @@ -301,6 +304,19 @@
> >
> >          ds.setIncludes(defaultPatterns.getIncludePatterns(p));
> >          ds.setExcludes(defaultPatterns.getExcludePatterns(p));
> > +        if (ds instanceof SelectorScanner) {
> > +            SelectorScanner ss = (SelectorScanner)ds;
> > +            ISelector[] selectors = getSelectors(p);
> > +            for (int i = 0; i < selectors.length; i++) {
> > +                selectors[i].assignBasedir(dir);
> > +                selectors[i].assignProject(p);
> > +                String msg = selectors[i].checkForError();
> > +                if (msg != null) {
> > +                    throw new BuildException(msg);
> > +                }
> > +            }
> > +            ss.setSelectors(selectors);
> > +        }
> >          if (useDefaultExcludes) {
> >            ds.addDefaultExcludes();
> >          }
> > Index: types/PatternSet.java
> > ===================================================================
> > RCS file:
> > /home/cvspublic/jakarta-ant/src/main/org/apache/tools/ant/types/Pa
> > tternSet.java,v
> > retrieving revision 1.19
> > diff -u -b -r1.19 PatternSet.java
> > --- types/PatternSet.java     26 Jan 2002 20:16:22 -0000      1.19
> > +++ types/PatternSet.java     4 Mar 2002 06:39:13 -0000
> > @@ -58,6 +58,8 @@
> >
> >  import org.apache.tools.ant.BuildException;
> >
> > +import org.apache.tools.ant.types.selectors.*;
> > +
> >  import java.io.File;
> >  import java.io.BufferedReader;
> >  import java.io.FileReader;
> > @@ -79,7 +81,7 @@
> >   * @author Jon S. Stevens <a
> > href="mailto:jon@clearink.com">jon@clearink.com</a>
> >   * @author <a href="mailto:stefan.bodewig@epost.de">Stefan Bodewig</a>
> >   */
> > -public class PatternSet extends DataType {
> > +public class PatternSet extends AndSelector {
> >      private Vector includeList = new Vector();
> >      private Vector excludeList = new Vector();
> >      private Vector includesFileList = new Vector();
> > @@ -332,6 +334,13 @@
> >                  createExclude().setName(excl[i]);
> >              }
> >          }
> > +
> > +        ISelector[] selectors = other.getSelectors(p);
> > +        if (selectors != null) {
> > +            for (int i=0; i<selectors.length; i++) {
> > +                appendSelector(selectors[i]);
> > +            }
> > +        }
> >      }
> >
> >      /**
> > @@ -347,7 +356,7 @@
> >      }
> >
> >      /**
> > -     * Returns the filtered include patterns.
> > +     * Returns the filtered exclude patterns.
> >       */
> >      public String[] getExcludePatterns(Project p) {
> >          if (isReference()) {
> > @@ -409,7 +418,7 @@
> >      }
> >
> >      /**
> > -     * Read includesfile ot excludesfile if not already done so.
> > +     * Read includesfile and excludesfile contents if not
> > already done so.
> >       */
> >      private void readFiles(Project p) {
> >          if (includesFileList.size() > 0) {
> > @@ -449,10 +458,13 @@
> >          }
> >      }
> >
> > -    public String toString()
> > -    {
> > -        return "patternSet{ includes: " + includeList +
> > -            " excludes: " + excludeList + " }";
> > +    public String toString() {
> > +        StringBuffer buf = new StringBuffer("patternSet{ includes: " +
> > +            includeList + " excludes: " + excludeList);
> > +        buf.append(super.toString());
> > +        buf.append(" }");
> > +
> > +        return buf.toString();
> >      }
> >
> >  }
> >
>
>
>--
>To unsubscribe, e-mail:   <mailto:ant-dev-unsubscribe@jakarta.apache.org>
>For additional commands, e-mail: <mailto:ant-dev-help@jakarta.apache.org>



--
To unsubscribe, e-mail:   <mailto:ant-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:ant-dev-help@jakarta.apache.org>


Mime
View raw message