Steve Appling wrote:
I have a git repo with a new Copy task that does not use Ant. It
allows us to determine if anything was really copied (getDidWork() is
accurate) and has some additional features from the current copy task.
This is in the copy branch at git://github.com/sappling/gradle.git
Hans has mentioned that he would like to get away from using Ant so
much internally, so I hope this will be useful.
The syntax changed some:
* it uses include and exclude (singular) instead of the previous
includes and excludes.
* it supports nested CopySpecs instead of the previous map of
includes/excludes per source dir (see javadoc for
org.gradle.api.tasks.copy.CopySpec)
* it supports regular expression based renaming
* it supports content filtering using java.io.FilterReader
This looks really good. I'm happy with the quality of the code. I have
quite a few comments below, but these are mainly around consistency with
the rest of the codebase, rather than the quality of the code. We can
address them, if we need to, after your changes have made it into the
trunk. There are, how a couple of things which I would like to see fixed
before we apply this:
- BreadthFirstDirectoryWalker and CopyVisitor really should have unit
tests. These are large classes, central to the whole thing. In
particular, some sad-day coverage would be good.
- The error paths need improvement. When CopyVisitor fails to copy a
file, it logs an error message and continues, and does not fail the
build. This is not good. It should throw an exception if it cannot copy
a file, and so fail the build. Also, CopySpecImpl.filter() both logs and
throws an exception. It should throw an exception and not log anything.
Some comments about the API. I don't really care if we address these
after your changes are applied, but we should try to do so before the
0.7 release (which is happening in the next few days):
- How do you copy a single file? How do you copy and rename a single file?
- CopySpec.from() methods should accept an Iterable rather than a List.
It would also be nice to offer ... methods as well.
- CopySpec.from() methods should accept exactly the same types as
Project.file(), and work the exactly the same way (for example, if you
pass in an absolute path as a String). Probably best if CopySpecImpl
simply delegates to a Project.file().
- I wonder if CopySpec.into() should only accept a String parameter, and
not accept a File parameter. The Copy task could continue to accept
into(File). It feels odd that I can have a nested CopySpec whose
destination directory is somewhere completely unrelated to the
destination directory of the outer CopySpec. Also, by accepting String
only, this makes CopySpec reusable for describing the contents of an
archive. Or a remote FTP directory. Or a VCS branch. Or pretty much any
target hierarchy.
- For the same reasons, remapTarget() would be better as a method which
operates on Strings (or possibly RelativePaths), rather than Files. For
example, we could have a rename(Closure) method: rename { String srcName
-> srcName.replaceAll('a', 'b') }.
- I don't like the semantics of rename(regexp, pattern). It should
either rename files, or it should filter files. It should not do both.
It should also operate on the full relative path of the source file,
rather than just it's base name. I'd be tempted to get rid of this
method, and replace it with the more general version above which takes a
Closure.
- I'd like to see tighter integration between Copy and
org.gradle.util.FileSet/PatternSet. For example, CopySpec should extend
PatternFilterable, and CopySpecImpl could use PatternSet to manage its
patterns.
- Copy.globalExcludes should be common to all things which do directory
scanning. It should probably be specified per-build, rather than
per-JVM. Perhaps it would be better as a property of Build, as a PatternSet?
Some comments on the implementation. These can be addressed later.
- CopySpecImpl.ant field doesn't seem to be used?
- A large number of the classes involved of the Copy task are reusable
outside the context of copying files, such as, DirectoryWalker + impl +
the pattern matching stuff. Also, we already have other places in the
code base which do directory scanning (for example, FileSet and the test
scanning). They really should make use of this new implementation. One
option is to move the scanning and pattern matching behind FileSet, and
add a visit(FileVisitor) method to FileSet. Then the Copy and Test tasks
can make use of FileSet. I'd also put the reusable classes in a package
other than o.g.api.internal.tasks.copy. Perhaps the same package as
FileSet would be more appropriate.
- We should reuse existing abstractions where possible: NameMapper could
be Transformer<String> and PatternMatcher could by Spec<RelativePath>
Some examples:
//basic copy is very similar to previous one
task(mydoc, type:Copy) {
from 'src/main/doc'
into 'build/target/doc'
exclude '**/*.bak'
}
// but each from starts a new CopySpec that inherits from the root one
task(initconfig, type:Copy) {
from('src/main/config') {
include '**\*.properties'
include '**\*.xml'
filter(ReplaceTokens, tokens:[version:'2.3.1'])
}
from('src/main/languages') {
rename 'EN_US_(*.)', '$1'
}
into 'build/target/config'
exclude '**\*.bak', '**\CVS\'
}
I have a few more things I would like to change if you want to use
this in 0.7. I would like to put some more details about it in the
userguide (looking for the best place)
Probably a new chapter which discusses dealing with files.
and I would like to add some type of static access to it. I find that
I do ant.copy all over the place inside other DefaultTasks.
This suggests to me that we're missing something. What do you use
ant.copy all over the place to actually do?
I can easily add a static accessor to do this, but this seems
generally useful for other tasks as well. Would anyone else like a
way to call all the types of tasks like this?
I'd be a bit reluctant to encourage this kind of scripting. You lose any
kind of declarative power when you use tasks like this in an imperative
way. I'd rather we made the model more descriptive so you don't need to
do this.
Having said that, there's probably a case to make for adding a file
manipulation DSL to let you easily deal with files. The Copy task could
then sit on top of this DSL, rather than being the DSL.
Adam
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
