-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix convention mapping compatibility for DirectoryProperty and RegularFileProperty #29180
Conversation
@bot-gradle test this |
I've triggered the following builds for you. Click here to see all build failures. |
A failure looks unrelated to changes |
@@ -193,6 +197,19 @@ public Provider<File> getAsFile() { | |||
return new MappingProvider<>(File.class, this, new ToFileTransformer()); | |||
} | |||
|
|||
@Override | |||
public void conventionFromAny(Provider<Object> file) { | |||
convention(file.map(any -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
convention(file.map(any -> { | |
convention(provider.map(value -> { |
This would make it more understandable for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My R$ 0,02 is that since file
reflects the role of the parameter, and provider
reflects the type, the former would be preferable (given that Java variables are explicitly typed). Maybe fileProvider
would be a decent middle ground. I am fine with any of the 3 options, just sharing an opinion about parameter and local variable naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both comments seem like a good improvements, thanks!
I also renamed method to conventionFromAnyFile
since we actually allow just some types of a file (either File or Directory) and not any object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that back, file
is also reflecting kind of type information (though something that is not available to the language), and conventionSource
or source
would be a better name for conveying its role. But again, I am fine with any of the options, just promoting the general principle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed that parameter to provider
so it's consisent with Property.convention(Provider<? extends T> provider)
.
I think readability won't change much if we rename it some more :)
@@ -99,7 +102,10 @@ private MappedProperty map(String propertyName, MappedPropertyImpl mapping) { | |||
} | |||
|
|||
private boolean mapConventionOn(SupportsConvention target, MappedPropertyImpl mapping) { | |||
if (target instanceof Property) { | |||
if (target instanceof DirectoryProperty || target instanceof RegularFileProperty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ use FileSystemLocationProperty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, fixed, thanks!
if (target instanceof Property) { | ||
if (target instanceof DirectoryProperty || target instanceof RegularFileProperty) { | ||
ConventionMappingFileSystemLocationPropertyProxy proxy = Cast.uncheckedNonnullCast(target); | ||
proxy.conventionFromAny(new DefaultProvider<>(() -> mapping.getValue(_convention, _source))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why wrap this in a Provider
? I think a Callable
is perfectly fine, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is beacuse convention on property doesn't support Callable (a missing feature?)
@NonNullApi | ||
public interface ConventionMappingFileSystemLocationPropertyProxy { | ||
void conventionFromAny(Provider<Object> file); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ I don't think we need this type. We can simply do what we need in ConventionAwareHelper.mapConventionOn()
when we detect a FileSystemLocationProperty
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this type because we cannot create a Directory or a RegularFile inside ConventionAwareHelper
, we can't access any factory there, or at least am not aware of a way to do that.
That is why we then pass File
to a DirectoryProperty/RegularFileProperty via this method and there we can then do a conversion from a File
to a Directory
or a RegularFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I noticed around, whenever we need implementations of an API type to support some internal protocol that we don't want to expose with API users, we create a SomeApiTypeInternal
interface that (optionally) extends SomeApiType
and is implemented by all implementations of SomeApiType
.
For instance, ProjectInternal
, PropertyInternal
, ProviderInternal
.
Would it make sense to follow that here, @asodja?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I introduced FileSystemLocationPropertyInternal extends FileSystemLocationProperty
. This also makes more sense now and it's more consisten with our code style compared to previous ConventionMappingFileSystemLocationPropertyProxy
.
private static abstract class AbstractFileVar<T extends FileSystemLocation, THIS extends FileSystemLocationProperty<T>> extends DefaultProperty<T> implements FileSystemLocationProperty<T> { | ||
private static abstract class AbstractFileVar<T extends FileSystemLocation, THIS extends FileSystemLocationProperty<T>> extends DefaultProperty<T> implements FileSystemLocationProperty<T>, ConventionMappingFileSystemLocationPropertyProxy { | ||
|
||
private final Class<T> type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an optimization? (given that the type is available via DefaultProperty.getType()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's not an optimization, I just didn't know that getType()
exists. Thanks for the insight, I will use that!
… FileSystemLocationPropertyInternal
@bot-gradle test this |
I've triggered the following builds for you. Click here to see all build failures. |
* Should be removed once ConventionMapping is removed. | ||
*/ | ||
@NonNullApi | ||
public interface FileSystemLocationPropertyInternal<T extends FileSystemLocation> extends FileSystemLocationProperty<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the convention is to also annotate the public type with @HasInternalProtocol
:
/**
* Indicates that there is an internal complementary protocol to the public type that is annotated with this.
*
* This should only be used on a type that is always assumed to also implement the internal protocol by Gradle internals.
*
* This exists to help anyone reading the source code realise that there is an internal component to the type.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation is for public types to warn users to not implement them. This type is internal anyway, so there is no added value to add this annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant to annotate the public type FileSystemLocationProperty
, not this internal protocol itself.
This annotation is for public types to warn users to not implement them.
Are you sure, Anže? The javadoc seems to describe it is meant exactly to denote the case we have here, a public type (FileSystemLocationProperty
) has an internal protocol (FileSystemLocationPropertyInternal
) that must be implemented by all implementations of the public type (AbstractFileVar
being the common ancestor to all of them):
This should only be used on a type that is always assumed to also implement the internal protocol by Gradle internals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense yes, I though you meant adding it to the new type. My bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #29177