Gradle › gradle-dev

Re: filesMatching - path to match.

Classic

List

Threaded

7 messages Options

Adam Murdoch

Re: filesMatching - path to match.

On 10 Feb 2014, at 4:04 pm, Luke Daley <[hidden email]> wrote:

Hi,

Would you expect the following to pass?

       given:
       file("a/b.txt") << "\$foo"

       when:
       buildScript """
           task c(type: Copy) {
               from("a") {
                   filesMatching("b.txt") {
                       expand foo: "bar"
                   }
                   into "nested"
               }
               into "out"
           }
       """

       then:
       succeeds "c"

       and:
       file("out/nested/b.txt").text == "bar"

It doesn't. It turns out that filesMatching matches the relative destination path, so it needs to be filesMatching("nested/b.txt"). I find this a little surprising.

It seems like it would be more predictable to use the relative path to the file from the nearest 'from' statement. What's the rationale for the current behaviour?

It’s just a bug, I would think. The pattern should be relative to the source path, not the destination path. An interesting question is what to do with those files whose name is transformed in some way (eg using rename()) - is the pattern applied before or after transformation?

Adam Murdoch
Gradle Co-founder
http://www.gradle.org
VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com

Luke Daley-2

Re: filesMatching - path to match.

> Adam Murdoch <mailto:[hidden email]>
> 13 February 2014 8:06 am
>
>
>
> It’s just a bug, I would think. The pattern should be relative to the
> source path, not the destination path. An interesting question is what
> to do with those files whose name is transformed in some way (eg
> using rename()) - is the pattern applied before or after transformation?
I can see cases where either would be the most convenient for the user.
However, I think using the original name is probably the easiest to
reason about and will yield the best predictability.

Raised http://issues.gradle.org//browse/GRADLE-3022

>
>
> --
> Adam Murdoch
> Gradle Co-founder
> http://www.gradle.org
> VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
> http://www.gradleware.com
>
>
>
> Luke Daley <mailto:[hidden email]>
> 11 February 2014 10:04 am
> Hi,
>
> Would you expect the following to pass?
>
> given:
> file("a/b.txt") << "\$foo"
>
> when:
> buildScript """
> task c(type: Copy) {
> from("a") {
> filesMatching("b.txt") {
> expand foo: "bar"
> }
> into "nested"
> }
> into "out"
> }
> """
>
> then:
> succeeds "c"
>
> and:
> file("out/nested/b.txt").text == "bar"
>
>
> It doesn't. It turns out that filesMatching matches the relative
> destination path, so it needs to be filesMatching("nested/b.txt"). I
> find this a little surprising.
>
> It seems like it would be more predictable to use the relative path to
> the file from the nearest 'from' statement. What's the rationale for
> the current behaviour?

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

Luke Daley-2

Re: filesMatching - path to match.

In reply to this post by Adam Murdoch

Adam Murdoch wrote:

>> Hi,
>>
>> Would you expect the following to pass?
>>
>> given:
>> file("a/b.txt") << "\$foo"
>>
>> when:
>> buildScript """
>> task c(type: Copy) {
>> from("a") {
>> filesMatching("b.txt") {
>> expand foo: "bar"
>> }
>> into "nested"
>> }
>> into "out"
>> }
>> """
>>
>> then:
>> succeeds "c"
>>
>> and:
>> file("out/nested/b.txt").text == "bar"
>>
>>
>> It doesn't. It turns out that filesMatching matches the relative
>> destination path, so it needs to be filesMatching("nested/b.txt"). I
>> find this a little surprising.
>>
>> It seems like it would be more predictable to use the relative path
>> to the file from the nearest 'from' statement. What's the rationale
>> for the current behaviour?
>
> It’s just a bug, I would think. The pattern should be relative to the
> source path, not the destination path. An interesting question is what
> to do with those files whose name is transformed in some way (eg
> using rename()) - is the pattern applied before or after transformation?

Looks like we explicitly wanted post rename:
https://github.com/gradle/gradle/blob/master/subprojects/core/src/integTest/groovy/org/gradle/api/tasks/CopyTaskIntegrationTest.groovy#L490

I think this is the wrong way to go. I can see that in some cases it is
useful, but I find it far harder to reason about than just applying to
the source path. Using the source location seems less surprising to me too.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

Luke Daley-2

Re: filesMatching - path to match.

In reply to this post by Adam Murdoch

Adam Murdoch wrote:

> Adam Murdoch wrote:
> - hide quoted text -- show quoted text -
>>> Hi,
>>>
>>> Would you expect the following to pass?
>>>
>>> given:
>>> file("a/b.txt") << "\$foo"
>>>
>>> when:
>>> buildScript """
>>> task c(type: Copy) {
>>> from("a") {
>>> filesMatching("b.txt") {
>>> expand foo: "bar"
>>> }
>>> into "nested"
>>> }
>>> into "out"
>>> }
>>> """
>>>
>>> then:
>>> succeeds "c"
>>>
>>> and:
>>> file("out/nested/b.txt").text == "bar"
>>>
>>>
>>> It doesn't. It turns out that filesMatching matches the relative
>>> destination path, so it needs to be filesMatching("nested/b.txt"). I
>>> find this a little surprising.
>>>
>>> It seems like it would be more predictable to use the relative path
>>> to the file from the nearest 'from' statement. What's the rationale
>>> for the current behaviour?
>>
>> It’s just a bug, I would think. The pattern should be relative to the
>> source path, not the destination path. An interesting question is
>> what to do with those files whose name is transformed in some way (eg
>> using rename()) - is the pattern applied before or after transformation?
>
> Looks like we explicitly wanted post rename:
> https://github.com/gradle/gradle/blob/master/subprojects/core/src/integTest/groovy/org/gradle/api/tasks/CopyTaskIntegrationTest.groovy#L490
>
>
> I think this is the wrong way to go. I can see that in some cases it
> is useful, but I find it far harder to reason about than just applying
> to the source path. Using the source location seems less surprising to
> me too.

Ok, now I'm not so sure. To break it down:

Source path matching:

* pro: predictable, easier to reason about
* pro: makes sense in nested specs
* con: doesn't chain
* con: can cause duplication if pulling files from multiple locations
* con: breaking change

Destination path matching:

* pro: supports chaining
* pro: simpler if destination is determining factor
* con: makes little sense in nested specs
* con: impossible to use source location as determining factor
* con: not the least surprising behaviour

I think the decisive factor for me is that using destination path
matching, using filesMatching in a nested spec is weird. Also, using
source path matching is more flexible.

I've got the change ready to go to move to source path matching, but
need some confirmation that we want to make the switch.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

Adam Murdoch

Re: filesMatching - path to match.

On 21 Feb 2014, at 11:02 am, Luke Daley <[hidden email]> wrote:

Adam Murdoch wrote:
Adam Murdoch wrote:
- hide quoted text -- show quoted text -
Hi,

Would you expect the following to pass?

      given:
      file("a/b.txt") << "\$foo"

      when:
      buildScript """
          task c(type: Copy) {
              from("a") {
                  filesMatching("b.txt") {
                      expand foo: "bar"
                  }
                  into "nested"
              }
              into "out"
          }
      """

      then:
      succeeds "c"

      and:
      file("out/nested/b.txt").text == "bar"

It doesn't. It turns out that filesMatching matches the relative destination path, so it needs to be filesMatching("nested/b.txt"). I find this a little surprising.

It seems like it would be more predictable to use the relative path to the file from the nearest 'from' statement. What's the rationale for the current behaviour?

It’s just a bug, I would think. The pattern should be relative to the source path, not the destination path. An interesting question is what to do with those files whose name is transformed in some way (eg using rename()) - is the pattern applied before or after transformation?

Looks like we explicitly wanted post rename: https://github.com/gradle/gradle/blob/master/subprojects/core/src/integTest/groovy/org/gradle/api/tasks/CopyTaskIntegrationTest.groovy#L490

I think this is the wrong way to go. I can see that in some cases it is useful, but I find it far harder to reason about than just applying to the source path. Using the source location seems less surprising to me too.

Ok, now I'm not so sure. To break it down:

Source path matching:

* pro: predictable, easier to reason about

I don’t think this one is really a pro for source path. I think it would go 50-50 source path to destination path if you surveyed people, possibly slightly in favour of destination.

* pro: makes sense in nested specs
* con: doesn't chain
* con: can cause duplication if pulling files from multiple locations
* con: breaking change

Destination path matching:

* pro: supports chaining
* pro: simpler if destination is determining factor
* con: makes little sense in nested specs
* con: impossible to use source location as determining factor
* con: not the least surprising behaviour

Another option is that we match on destination path relative to the containing copy spec’s destination. This leaves the pros and removes one of the cons. This is how other paths are interpreted in a copy spec.

If someone comes up with a good use case for source location (I don’t have one) then we can add more stuff to FileCopyDetails and you can use eachFile { … }.

Or, we might make this destination vs source path distinction explicit. We need a decent predicate DSL for our model DSL anyway. A mock up:

filesMatching(dest: ‘a/b/c’) { … }

filesMatching(sourcePath: ‘**/*.java’) { … }

or even

eachFile(dest: ‘a/b/c’) { … }

eachFile(dest: !’a/b/c’) { … }

Adam Murdoch
Gradle Co-founder
http://www.gradle.org
VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com

Luke Daley-2

Re: filesMatching - path to match.

> Adam Murdoch <mailto:[hidden email]>
> 4 March 2014 8:37 am
>
> On 21 Feb 2014, at 11:02 am, Luke Daley <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>>
>>
>> Adam Murdoch wrote:
>>> Adam Murdoch wrote:
>>> - hide quoted text -- show quoted text -
>>>>> Hi,
>>>>>
>>>>> Would you expect the following to pass?
>>>>>
>>>>> given:
>>>>> file("a/b.txt") << "\$foo"
>>>>>
>>>>> when:
>>>>> buildScript """
>>>>> task c(type: Copy) {
>>>>> from("a") {
>>>>> filesMatching("b.txt") {
>>>>> expand foo: "bar"
>>>>> }
>>>>> into "nested"
>>>>> }
>>>>> into "out"
>>>>> }
>>>>> """
>>>>>
>>>>> then:
>>>>> succeeds "c"
>>>>>
>>>>> and:
>>>>> file("out/nested/b.txt").text == "bar"
>>>>>
>>>>>
>>>>> It doesn't. It turns out that filesMatching matches the relative
>>>>> destination path, so it needs to be filesMatching("nested/b.txt").
>>>>> I find this a little surprising.
>>>>>
>>>>> It seems like it would be more predictable to use the relative
>>>>> path to the file from the nearest 'from' statement. What's the
>>>>> rationale for the current behaviour?
>>>>
>>>> It’s just a bug, I would think. The pattern should be relative to
>>>> the source path, not the destination path. An interesting question
>>>> is what to do with those files whose name is transformed in some
>>>> way (eg using rename()) - is the pattern applied before or after
>>>> transformation?
>>>
>>> Looks like we explicitly wanted post rename:
>>> https://github.com/gradle/gradle/blob/master/subprojects/core/src/integTest/groovy/org/gradle/api/tasks/CopyTaskIntegrationTest.groovy#L490
>>>
>>>
>>> I think this is the wrong way to go. I can see that in some cases it
>>> is useful, but I find it far harder to reason about than just
>>> applying to the source path. Using the source location seems less
>>> surprising to me too.
>>
>> Ok, now I'm not so sure. To break it down:
>>
>> Source path matching:
>>
>> * pro: predictable, easier to reason about
>
> I don’t think this one is really a pro for source path. I think it
> would go 50-50 source path to destination path if you surveyed people,
> possibly slightly in favour of destination.

The basis of my argument here is that froms don't nest.

>> * pro: makes sense in nested specs
>> * con: doesn't chain
>> * con: can cause duplication if pulling files from multiple locations
>> * con: breaking change
>>
>> Destination path matching:
>>
>> * pro: supports chaining
>> * pro: simpler if destination is determining factor
>> * con: makes little sense in nested specs
>> * con: impossible to use source location as determining factor
>> * con: not the least surprising behaviour
>
> Another option is that we match on destination path relative to the
> containing copy spec’s destination. This leaves the pros and removes
> one of the cons. This is how other paths are interpreted in a copy spec.
>
> If someone comes up with a good use case for source location (I don’t
> have one) then we can add more stuff to FileCopyDetails and you can
> use eachFile { … }.
>
> Or, we might make this destination vs source path distinction
> explicit. We need a decent predicate DSL for our model DSL anyway. A
> mock up:
>
> filesMatching(dest: ‘a/b/c’) { … }
> filesMatching(sourcePath: ‘**/*.java’) { … }

And deprecate filesMatching(String) variant?

Supporting both makes good sense I think. I'd prefer not to use “named
params” though… ever.
>
> or even
>
> eachFile(dest: ‘a/b/c’) { … }
> eachFile(dest: !’a/b/c’) { … }
We'd have to use a transform to support being able to use ! like you are
here. I'm strongly against this.

An alternative not involving a transform would be to not use strings
here but some stronger custom type for path patterns. '!' is an
overloadable operator in Groovy.

>
>
> --
> Adam Murdoch
> Gradle Co-founder
> http://www.gradle.org
> VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
> http://www.gradleware.com
>
>
>
> Luke Daley <mailto:[hidden email]>
> 21 February 2014 10:02 am
>
>
>
>
> Ok, now I'm not so sure. To break it down:
>
> Source path matching:
>
> * pro: predictable, easier to reason about
> * pro: makes sense in nested specs
> * con: doesn't chain
> * con: can cause duplication if pulling files from multiple locations
> * con: breaking change
>
> Destination path matching:
>
> * pro: supports chaining
> * pro: simpler if destination is determining factor
> * con: makes little sense in nested specs
> * con: impossible to use source location as determining factor
> * con: not the least surprising behaviour
>
>
> I think the decisive factor for me is that using destination path
> matching, using filesMatching in a nested spec is weird. Also, using
> source path matching is more flexible.
>
> I've got the change ready to go to move to source path matching, but
> need some confirmation that we want to make the switch.
>
>
>
>
> Adam Murdoch <mailto:[hidden email]>
> 13 February 2014 8:06 am
>
>
>
> It’s just a bug, I would think. The pattern should be relative to the
> source path, not the destination path. An interesting question is what
> to do with those files whose name is transformed in some way (eg
> using rename()) - is the pattern applied before or after transformation?
>
>
> --
> Adam Murdoch
> Gradle Co-founder
> http://www.gradle.org
> VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
> http://www.gradleware.com
>
>
>
> Luke Daley <mailto:[hidden email]>
> 11 February 2014 10:04 am
> Hi,
>
> Would you expect the following to pass?
>
> given:
> file("a/b.txt") << "\$foo"
>
> when:
> buildScript """
> task c(type: Copy) {
> from("a") {
> filesMatching("b.txt") {
> expand foo: "bar"
> }
> into "nested"
> }
> into "out"
> }
> """
>
> then:
> succeeds "c"
>
> and:
> file("out/nested/b.txt").text == "bar"
>
>
> It doesn't. It turns out that filesMatching matches the relative
> destination path, so it needs to be filesMatching("nested/b.txt"). I
> find this a little surprising.
>
> It seems like it would be more predictable to use the relative path to
> the file from the nearest 'from' statement. What's the rationale for
> the current behaviour?

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

Adam Murdoch

Re: filesMatching - path to match.

On 4 Mar 2014, at 9:53 am, Luke Daley <[hidden email]> wrote:

Adam Murdoch <[hidden email]>
4 March 2014 8:37 am

On 21 Feb 2014, at 11:02 am, Luke Daley <[hidden email] <[hidden email]>> wrote:

Adam Murdoch wrote:
Adam Murdoch wrote:
- hide quoted text -- show quoted text -
Hi,

Would you expect the following to pass?

     given:
     file("a/b.txt") << "\$foo"

     when:
     buildScript """
         task c(type: Copy) {
             from("a") {
                 filesMatching("b.txt") {
                     expand foo: "bar"
                 }
                 into "nested"
             }
             into "out"
         }
     """

     then:
     succeeds "c"

     and:
     file("out/nested/b.txt").text == "bar"

It doesn't. It turns out that filesMatching matches the relative destination path, so it needs to be filesMatching("nested/b.txt"). I find this a little surprising.

It seems like it would be more predictable to use the relative path to the file from the nearest 'from' statement. What's the rationale for the current behaviour?

It’s just a bug, I would think. The pattern should be relative to the source path, not the destination path. An interesting question is what to do with those files whose name is transformed in some way (eg using rename()) - is the pattern applied before or after transformation?

Looks like we explicitly wanted post rename: https://github.com/gradle/gradle/blob/master/subprojects/core/src/integTest/groovy/org/gradle/api/tasks/CopyTaskIntegrationTest.groovy#L490

I think this is the wrong way to go. I can see that in some cases it is useful, but I find it far harder to reason about than just applying to the source path. Using the source location seems less surprising to me too.

Ok, now I'm not so sure. To break it down:

Source path matching:

* pro: predictable, easier to reason about

I don’t think this one is really a pro for source path. I think it would go 50-50 source path to destination path if you surveyed people, possibly slightly in favour of destination.
The basis of my argument here is that froms don't nest.
* pro: makes sense in nested specs
* con: doesn't chain
* con: can cause duplication if pulling files from multiple locations
* con: breaking change

Destination path matching:

* pro: supports chaining
* pro: simpler if destination is determining factor
* con: makes little sense in nested specs
* con: impossible to use source location as determining factor
* con: not the least surprising behaviour

Another option is that we match on destination path relative to the containing copy spec’s destination. This leaves the pros and removes one of the cons. This is how other paths are interpreted in a copy spec.

If someone comes up with a good use case for source location (I don’t have one) then we can add more stuff to FileCopyDetails and you can use eachFile { … }.

Or, we might make this destination vs source path distinction explicit. We need a decent predicate DSL for our model DSL anyway. A mock up:

filesMatching(dest: ‘a/b/c’) { … }
filesMatching(sourcePath: ‘**/*.java’) { … }
And deprecate filesMatching(String) variant?

Supporting both makes good sense I think. I'd prefer not to use “named params” though… ever.

Sure. I’m not particularly attached to this particular dsl - I just want some consistent and concise dsl for matching things.

or even

eachFile(dest: ‘a/b/c’) { … }
eachFile(dest: !’a/b/c’) { … }
We'd have to use a transform to support being able to use ! like you are here. I'm strongly against this.

Sure. It’s only to illustrate the idea. We will have to deal with negation in some general way in the model rules DSL, and whatever we come up with there can be applied here.

Adam Murdoch
Gradle Co-founder
http://www.gradle.org
VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com