Adam Murdoch wrote:
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.
ring using java.io.FilterReader
clip ...
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:
I can work on these issues over the next few days.
- 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.
Yes
- 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.
Yes
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?
I apparently haven't thought this use case through. I'll look into it.
- CopySpec.from() methods should accept an Iterable rather than a List.
It would also be nice to offer ... methods as well.
Yes
- 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().
Yes
- 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') }.
Let me think about these two
- 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 had conflicting desires with this one. I actually have use cases both ways.
Let me re-visit my current use of this in our build. It is implemented this way
largely because it made our main use case easier. Looking at it now, however, I
don't think this would be obvious behavior. If I still feel that I have a use
case that needs this, perhaps the default behavior can be to just rename and an
optional parameter (like "allowOnlyMatching") can turn on this other behavior.
I think my original problem may have been that I couldn't do regular expression
based excludes, so I couldn't get the same result without this. At some point
it may be good to have a regexp syntax for includes/excludes, but let's wait on
that.
- 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?
I would like to see tighter integration here too. I intended the pattern
matching and directory walker features to be reused. I think they can be handy
with many other parts of the system, but I intentionally restricted my changes
to the Copy task until I found out how well it would be received.
Some comments on the implementation. These can be addressed later.
- CopySpecImpl.ant field doesn't seem to be used?
This is a hold over from the time when I was using the Groovy Ant FileScanner to
do the pattern matching. It should be removed now. I will say that the pattern
matching algorithm in DefaultPatternMatcher is the part of this code that I was
least confident in. We have been using this copy code internally for a couple
of months and I have run into pattern matching bugs here several times. All of
these issues have been corrected and it passes a wide range of pattern matching
tests, but writing a good pattern matcher was more complicated than I originally
expected. I probably left the ant field in CopySpecImpl because I was worried
that someone would suggest that I just go back to using FileScanner:)
- 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.
I was just trying to keep this self contained until I saw how it was excepted.
I will move it to a more re-usable package.
- We should reuse existing abstractions where possible: NameMapper could
be Transformer<String> and PatternMatcher could by Spec<RelativePath>
I'll look into this. I am not as aware of some of these abstractions, but these
seem to be good suggestions to me.
clip ...
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.
Have you considered breaking the userguide into two parts (not for 0.7). I
think this might be more easily consumed if there was both a tutorial and a
separate reference doc. This is probably a discussion for a thread in the users
list, but I have found the current userguide a little bit unsatisfying for both
of these purposes. I think it might be helped by breaking apart these two intents.
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?
The same type of things that Gradle does with it's build file. Gradle's
build.gradle uses ant.copy 9 times. There are many times that what seems like a
single task to a user involves some combination of deleting, jarring, signing,
and copying files. It is possible to do this as multiple tasks chained together
via dependencies, but none of those smaller tasks would do anything useful by
themselves. For example, Gradle's "userguidePdf" task could be broken into
generateUserGuidePdfSourceHTML, copyUserGuideStyle, copyUserGuidePrint, and
transformUserGuideToPDF tasks that are much more declarative, but (in my
perspective) harder to read and maintain. I don't see the benefit of breaking
these up. I believe that it is very common to have single tasks that do several
steps using ant right now. As we expose new tasks that don't use ant, I think
they should be able to be used in this way as well.
Exposing this as a CopyAction that you can access without a Copy task lets you
still check if copyAction.didWork so that you can appropriately set didWork for
your DefaultTask. (I don't know if you wrote this before I actually implemented
CopyAction, but you might look at the current
CopyTaskIntegrationTest.testCopyAction to see an example).
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
Adam, thanks for taking the time to review this and pull together all of these
details. I don't have enough breadth of knowledge of Gradle to see some of
these issues.
--
Steve Appling
Automated Logic Research Team
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
