Eclipse resource filters #846
Conversation
|
Some notes:
|
| @@ -242,6 +259,63 @@ private void addLinkedResourcesToXml() { | |||
| } | |||
| } | |||
|
|
|||
| private void addResourceFiltersToXml() { | |||
| // other nodes like linkedResources are appended even if they are empty, but as | |||
There was a problem hiding this comment.
I wouldn't change the behavior compared to the other ones here.
There was a problem hiding this comment.
Ok, I will update the old failing tests that were not expecting this empty node.
|
|
||
| private int getResourceFilterType(ResourceFilter resourceFilter) { | ||
| int type = 0; | ||
| if (resourceFilter == null) { |
There was a problem hiding this comment.
Ok, I will pick and document some default values and prevent nulls in the ResourceFilter setters.
| if (resourceFilter == null) { | ||
| return type; | ||
| } | ||
| if (resourceFilter.getType() != null) { |
There was a problem hiding this comment.
null should not be allowed in the first place
| break; | ||
| } | ||
| } | ||
| if (resourceFilter.getAppliesTo() != null) { |
There was a problem hiding this comment.
null should not be allowed in the first place
| break; | ||
| } | ||
| } | ||
| if (resourceFilter.getRecursive() != null && resourceFilter.getRecursive()) { |
There was a problem hiding this comment.
null should not be allowed in the first place
| * | ||
| * @since NEXT_VERSION | ||
| */ | ||
| public class ResourceFilterMatcher extends GroovyObjectSupport { |
There was a problem hiding this comment.
Do not extend GroovyObjectSupport. This is only there in other classes for backwards compatibility and not needed for newly added classes.
| void assertHasReferencedProjects(String... referencedProjects) { | ||
| assert this.project.projects.project*.text() == referencedProjects as List | ||
| } | ||
|
|
||
| void assertHasResourceFilterXml(String expectedFilteredResourcesXml) { |
There was a problem hiding this comment.
Let's move these methods into a FilteredResourcesFixture
There was a problem hiding this comment.
Now that the ids are sequential and predictable, most of this code is now unnecessary as we can just put the expected id in the xml. I will remove it.
| public Set<ResourceFilter> getResourceFilters() { | ||
| return resourceFilterSet; | ||
| } | ||
| // setResourceFilters is not supported because ResourceFilterSet is a special DSL type |
There was a problem hiding this comment.
Remove implementation comment. This is obvious from the type signature.
| @@ -124,6 +124,8 @@ | |||
|
|
|||
| private Set<Link> linkedResources = Sets.newLinkedHashSet(); | |||
|
|
|||
| private ResourceFilterSet resourceFilterSet = new ResourceFilterSet(); | |||
There was a problem hiding this comment.
Let's do the same as for linkedResources: Have the resourceFilter method directly on EclipseProject and get rid of the additional nesting level in the DSL. Then you also no longer need ResourceFilterSet
eclipse.project {
resourceFilter {
type = ...
}
}
instead of the current
eclipse.project {
resourceFilters {
filter {
type = ...
}
}
}
| * | ||
| * @since NEXT_VERSION | ||
| */ | ||
| public class ResourceFilter extends GroovyObjectSupport { |
There was a problem hiding this comment.
Do not extend GroovyObjectSupport
|
Hi @oehme , I have addressed your comments with a new commit. We can squash later before merging. I will work on the reading of the existing .project filteredResources next and add as a new commit here. |
|
Hi @oehme , I have now added the support for reading filteredResources from an existing .project file and documentation for the new features. Would you please take another look?
|
| /** | ||
| * The gradle DSL model of an Eclipse resource filter. | ||
| * | ||
| * Later this will be extended with additional helpers, but for now it just allows specifying a filter with a custom matcher |
There was a problem hiding this comment.
Let's not talk about the future here, we never know when we get around to it :)
| * | ||
| * eclipse { | ||
| * project { | ||
| * // the following closure configures the ResourceFilter |
There was a problem hiding this comment.
I think the code is pretty self-explanatory, no comment required
| * type = 'EXCLUDE_ALL' | ||
| * matcher { | ||
| * id = 'org.eclipse.ui.ide.multiFilter' | ||
| * arguments = '1.0-name-matches-false-false-node_modules' |
There was a problem hiding this comment.
This might need some explanation, like "to find out which arguments to use, configure the desired filter with Eclipse's UI and copy the argument String over"
| private boolean recursive = true; | ||
| private ResourceFilterMatcher matcher; | ||
|
|
||
| // Needed for DSL configuration Actions |
There was a problem hiding this comment.
Ok, will remove the comment. I was trying to find a way to make sure the class was in a valid state by validating the setters and requiring a constructor with all arguments, but this doesn't seem to work well with configuration by closure/Action.
There was a problem hiding this comment.
Yes, we can't validate this up-front, we can only give reasonable defaults (or fail later on if the user forgot something)
| } | ||
|
|
||
| /** | ||
| * Sets the ResourceFilterAppliesTo |
There was a problem hiding this comment.
should have the same documentation as the getter
There was a problem hiding this comment.
Ok, copied the getter documentation to the setter also
| /** | ||
| * The model of an Eclipse resource filter matcher. | ||
| * <p> | ||
| * The matcher configures the behavior of its parent ResourceFilter. The matcher configures |
There was a problem hiding this comment.
The matcher does not configure the behavior. The matcher decides when the containing filter (or containing matcher) applies.
| private String arguments; | ||
| private Set<ResourceFilterMatcher> children = Sets.newLinkedHashSet(); | ||
|
|
||
| // Needed for DSL configuration Actions |
|
Ok, I have added test "existing project file with equivalent resource filters is unchanged" to test that an existing .project file with two resource filters is unchanged after the "eclipse" task is run on a gradle project that defines two equivalent filters. One thing to note is that the ids of the filters will possibly change if Eclipse had generated the filters, as it will use a timestamp instead of sequential integers. As we do not store the id in our model, we can't recreate it, so the id will just change to a sequential number. But subsequent runs of the "eclipse" task will keep the same filter ordering and numbering. Do you think this will be an issue? |
|
Hi @oehme , I have completed the latest round of code review changes, would you please have another look? Thanks! |
I think we can live with that, yes. Can you check that Eclipse doesn't change the id back when you import the project? Otherwise it could become an annoying back-and-forth. Apart from that, this looks really great to me and I can merge it as soon as the question above is clarified. |
kpage
force-pushed
the
eclipse-resource-filters
branch
from
43dbc20
to
06ce74d
Compare
|
Hi @oehme , I tested importing the generated .project file into Eclipse, and Eclipse does not change the identifiers. It will only change the ids if the filters are modified in the UI, and in that case it rewrites all of them (sometimes the ids are actually not unique, they are the same exact timestamp...). So I think this behavior is fine. I have squashed the code review commits and rebased onto the latest master, you can merge when you are ready. |
|
Thanks for the great work @kpage! I'll be out for a week, but will merge this when I'm back. It will be part of Gradle 3.4, so if you have some time to adjust the "NEXT_VERSION" tags that would help. |
kpage
force-pushed
the
eclipse-resource-filters
branch
from
06ce74d
to
7318dcb
Compare
|
Hi @oehme , I changed the NEXT_VERSIONs to 3.4 and rebased onto latest master. |
|
Hi @kpage. This change looks good but we do have an outstanding test failure on Windows Failure detailsCondition not satisfied:projectFileOriginalText == projectFile.text ------- Stdout: ------- :eclipseJdt :eclipseProject :eclipse BUILD SUCCESSFUL Total time: 0.089 secs ------- Stderr: ------- Warning: org.apache.xerces.parsers.SAXParser: Property 'http://www.oracle.com/xml/jaxp/properties/entityExpansionLimit' is not recognized. |
eriwen
added
waiting-for:contributor
and removed
a:feature
|
|
||
| ''' | ||
| project.assertHasResourceFilterXml(resourceFilterXml) | ||
| projectFileOriginalText == projectFile.text |
There was a problem hiding this comment.
This is the reason for the test failure. You need to normalize line separators if you want to compare a String and the content from a file.
kpage
force-pushed
the
eclipse-resource-filters
branch
from
7318dcb
to
43e47fa
Compare
|
@kpage Yeah, I kicked off CI for your PR here: https://builds.gradle.org/viewQueued.html?itemId=2197344&tab=queuedBuildOverviewTab I'm afraid this didn't make it into 3.4. Could you please update the |
From buildship eclipse bug report 495799.
kpage
force-pushed
the
eclipse-resource-filters
branch
from
43e47fa
to
623c90c
Compare
|
Hi @eriwen , I have updated the new |
|
@kpage Would you like to follow up with the Tooling API part next so we have a complete story for 3.5? |
| * | ||
| * @param configureClosure The closure to use to configure the resource filter. | ||
| */ | ||
| public ResourceFilter resourceFilter(@DelegatesTo(value=ResourceFilter.class, strategy = Closure.DELEGATE_FIRST) Closure configureClosure) { |
There was a problem hiding this comment.
We don't need to add Closure taking methods. Just the Action method is enough.
| * | ||
| * @param configureClosure The closure to use to configure the matcher. | ||
| */ | ||
| public ResourceFilterMatcher matcher(@DelegatesTo(value=ResourceFilterMatcher.class, strategy = Closure.DELEGATE_FIRST) Closure configureClosure) { |
| * | ||
| * @param configureClosure The closure to use to configure the matcher. | ||
| */ | ||
| public ResourceFilterMatcher matcher(@DelegatesTo(value=ResourceFilterMatcher.class, strategy = Closure.DELEGATE_FIRST) Closure configureClosure) { |
| return children; | ||
| } | ||
|
|
||
| public void setChildren(Set<ResourceFilterMatcher> children) { |
There was a problem hiding this comment.
Is there an example of when this would be useful?
One weird thing about the way this is done is matcher {} at this level adds a new matcher, but matcher {} in the resource filter configures an existing one.
There was a problem hiding this comment.
Hi @big-guy , are you asking why nested ResourceFilterMatchers are supported?
This is required by the existing Eclipse model of filters/matchers. Nested matchers are actually a pretty common use case. Suppose you wanted to filter out either of two folders from showing in Eclipse, this would be modeled as an or matcher with nested matchers for the folder names:
eclipse {
project {
resourceFilter {
matcher {
id = 'org.eclipse.ui.ide.orFilterMatcher'
matcher {
id = 'org.eclipse.ui.ide.multiFilter'
arguments = '1.0-name-matches-false-false-node_modules'
}
matcher {
id = 'org.eclipse.ui.ide.multiFilter'
arguments = '1.0-name-matches-false-false-build'
}
}
}
}
}
Regarding the DSL issue you mention where matcher at the filter level creates/configures a single matcher whereas matcher at the matcher level creates a new nested matcher, that is something I struggled with as well.
I took a look at other examples in the gradle code where the DSL configures single objects vs. collections of objects, and this seemed to be how it was done elsewhere (EclipseProject.linkedResource, for example, creates a new Link and adds to the set whereas EclipseClasspath.file configures the single XmlFileContentMerger).
The confusion here just seems to stem from the required domain complexity that a filter can have exactly one matcher but matchers themselves can have multiple nested matchers. I guess the method names could be differentiated (matcher vs nestedMatcher?), but that might also cause another confusion as to why the same object type is constructed with different keywords depending on its placement in the DSL tree. Let me know if you have any suggestions.
Please keep in mind that the current implementation is just the "raw" domain model that gets gradle to parity with the Eclipse resource filters, but @oehme has spec'd a much more intuitive extension to the DSL that can be layered on top of this.
| private String arguments; | ||
| private Set<ResourceFilterMatcher> children = Sets.newLinkedHashSet(); | ||
|
|
||
| public ResourceFilterMatcher() { |
There was a problem hiding this comment.
Several of these new classes have 2 constructors, but it seems like only one of them is useful/used often?
There was a problem hiding this comment.
The default constructor was required to initialize an empty object that is configured by the Action while the full constructor is useful to construct instances for tests (ProjectTest, ResourceFilterTest, ResourceFilterMatcherTest). Please let me know if you have a more idiomatic way of constructing the DSL objects for Actions, etc.
| this.children = children; | ||
| } | ||
|
|
||
| public String getId() { |
There was a problem hiding this comment.
This isn't marked as @Nullable, but it seems like we expect it could be null sometimes (see hashCode)?
There was a problem hiding this comment.
The id is required by Eclipse for the matcher to work, I could add validation logic in setId to throw InvalidUserDataException if null is passed like we do for "children". I added the null check in hashCode because these are mutable DSL objects that are constructed empty and then populated later by Action or from reading an existing .project file, so it is possible that hashCode might be called and throw NPE before the object is fully/correctly configured. EqualsVerifier pointed out the NPE when I was testing.
There was a problem hiding this comment.
Null check on setter added in #1328
| @@ -30,6 +30,7 @@ dependencies { | |||
| compile libraries.inject | |||
|
|
|||
| testCompile libraries.xmlunit | |||
| testCompile libraries.equalsVerifier | |||
There was a problem hiding this comment.
This is a nit-picky thing, but we only use this in two tests and I think we could capture about the same thing by checking that the effect of a bad equals/hashCode method is caught in the integration tests (for the generated Eclipse metadata files).
There was a problem hiding this comment.
Ok, I had added this for myself to catch errors with the manually written equals/hashCode required by the DSL and decided to leave it in case the gradle team found it useful.
I've found EqualsVerifier quite useful on other projects as a low-effort way to catch issues with Java hashCode/equals (it catches things like possible NPEs, fields that were recently added without updating hashCode/equals, using a field in hashCode but forgetting it in equals, the semantics of getClass() vs instanceof in the presence of inheritance, and a few other useful things). To check these edge cases in the integration tests might run over 1000 more lines of test code and take longer to execute.
If you think you will not use it often, I can remove it. Please let me know what you decide.
Hi @oehme ! This is a draft pull request for the first phase of the eclipse resource filters, adding the EclipseProject DSL support for filters with a basic custom matcher. I am still cleaning up the documentation, but I wanted to open this so you could start to take a look.
Any of the checked boxes below indicate that I took action:
./gradlew quickCheckFor all non-trivial changes that modify the behavior or public API:
test coverage to verify the behavior. Before submitting the pull request
I ran a build on your local machine via the command
./gradlew quickCheck <impacted-subproject>:check.DSL reference and Javadocs where applicable.