You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/06/01 10:09:24 UTC
[GitHub] [maven-dependency-plugin] bmarwell opened a new pull request #135: [MDEP-568] go offline filter
bmarwell opened a new pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MDEP) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[MDEP-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MDEP-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] elharo commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643228107
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
Review comment:
It tends to force methods to be public for use by other subpackages that otherwise wouldn't have to be public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] antoniolucasnobar commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
antoniolucasnobar commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r674065579
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
Review comment:
@elharo is this change a requirement for this PR to be accepted? I agree with @bmarwell approach (to be future proof), but if it is mandatory, I think it is possible to change packages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643453846
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/ArtifactIdFilter.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Filters dependencies by artifact ID.
+ */
+public class ArtifactIdFilter extends AbstractDependencyFilter
+{
+ private final String includeArtifactId;
+
+ private final String excludeArtifactId;
+
+ public ArtifactIdFilter( String includeArtifactId, String excludeArtifactId )
Review comment:
The documentation does not state what should happen if it is in both lists. In this case it will be excluded. That should be fine and is not surprising behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] antoniolucasnobar commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
antoniolucasnobar commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r666972669
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+
+abstract class AbstractDependencyFilter implements DependencyFilter
Review comment:
Hello @bmarwell ,
Since you created this abstract class, maybe it could contain more than just `splitValues` method. The inherited classes are very similar IMO; This way, this abstract class could have almost all logic here, with the filter, include and exclude methods; It is just a suggestion to avoid code duplication.
Below an example of the abstract class and one of its child. Even the `filterexcludeIds` and `filterincludeIds` could be simplified because they share similar logic.
As ScopeFilter has specific treatment to split include and exclue ids, I created two functions `splitIncludeIds` and `splitExcludeIds`
```java
abstract class AbstractDependencyFilter implements DependencyFilter
{
protected final String includeIds;
protected final String excludeIds;
public AbstractDependencyFilter(String includeIds, String excludeIds)
{
this.includeIds = includeIds == null ? "" : includeIds;
this.excludeIds = excludeIds == null ? "" : excludeIds;
}
@Override
public Set<Dependency> filter(Set<Dependency> dependencies)
{
Set<Dependency> filtered = new HashSet<>(dependencies);
filtered = filterincludeIds(filtered);
filtered = filterexcludeIds(filtered);
return filtered;
}
private Set<Dependency> filterexcludeIds(Set<Dependency> dependencies)
{
if (excludeIds.trim().isEmpty()) {
return dependencies;
}
final Set<String> excludedIds = splitExcludeIds(excludeIds);
Set<Dependency> filtered = new HashSet<>(dependencies.size());
for (Dependency dependency : dependencies) {
if (excludedIds.contains(getContainsProperty(dependency))) {
continue;
}
filtered.add(dependency);
}
return filtered;
}
private Set<Dependency> filterincludeIds(Set<Dependency> dependencies)
{
if (includeIds.trim().isEmpty()) {
return dependencies;
}
Set<String> includedIds = splitIncludeIds(includeIds);
Set<Dependency> filtered = new HashSet<>(dependencies.size());
for (Dependency dependency : dependencies) {
if (includedIds.contains(getContainsProperty(dependency))) {
filtered.add(dependency);
}
}
return filtered;
}
protected Set<String> splitExcludeIds(String excludeIds) {
return splitValues(excludeIds);
}
protected Set<String> splitIncludeIds(String includeIds) {
return splitValues(includeIds);
}
protected Set<String> splitValues(String csvValueList) {
// ommited here
}
abstract protected String getContainsProperty(Dependency dependency);
}
```
```java
public class GroupIdFilter extends AbstractDependencyFilter
{
public GroupIdFilter(String includeGroupIds, String excludeGroupIds)
{
super(includeGroupIds, excludeGroupIds);
}
protected String getContainsProperty(Dependency dependency) {
return dependency.getGroupId();
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643453176
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/ArtifactIdFilter.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Filters dependencies by artifact ID.
+ */
+public class ArtifactIdFilter extends AbstractDependencyFilter
+{
+ private final String includeArtifactId;
+
+ private final String excludeArtifactId;
+
+ public ArtifactIdFilter( String includeArtifactId, String excludeArtifactId )
Review comment:
Another copy/paste from other parts of the mojo. We have the choice of consistency vs doing it right.
Those come from the mojo as string parameter, thus can't change it. Have to split it here. But will change to plural.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] elharo commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643041054
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
Review comment:
Is this introducing a new package? Probably not needed.
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+abstract class AbstractDependencyFilter implements DependencyFilter
+{
+ public boolean isDependencyIncluded( Dependency dependency )
Review comment:
Use `@Override` where appropriate
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/ClassifierFilter.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Filters dependencies by Classifier.
Review comment:
classifier (lower case)
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/FilterDependencies.java
##########
@@ -0,0 +1,91 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.unmodifiableList;
+
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Applies null to any number of filters to a given collection of dependencies.
Review comment:
I can't quite parse this comment.
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/FilterDependencies.java
##########
@@ -0,0 +1,91 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.unmodifiableList;
+
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Applies null to any number of filters to a given collection of dependencies.
+ */
+public class FilterDependencies
+{
+
+ private final List<DependencyFilter> filters;
+
+ /**
+ * Created new instance.
+ */
+ public FilterDependencies( DependencyFilter... filters )
+ {
+ this.filters = unmodifiableList( asList( filters ) );
+ }
+
+ /**
+ * <p>filter.</p>
+ *
+ * @param dependencies The {@link Dependency}s to filter.
+ * @return The resulting artifacts set.
+ */
+ public Set<Dependency> filter( Collection<Dependency> dependencies )
+ {
+ Set<Dependency> filtered = new HashSet<>( dependencies );
+
+ for ( DependencyFilter filter : filters )
+ {
+ try
+ {
+ filtered = filter.filter( filtered );
+ }
+ catch ( NullPointerException e )
+ {
+ // TODO: log
Review comment:
what an NPE here imply? why not let it bubble up?
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/GroupIdFilter.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Filters dependencies by GroupId.
Review comment:
group ID.
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+abstract class AbstractDependencyFilter implements DependencyFilter
+{
+ public boolean isDependencyIncluded( Dependency dependency )
+ {
+ Set<Dependency> set = new LinkedHashSet<>();
+ set.add( dependency );
+
+ set = filter( set );
+ return set.contains( dependency );
Review comment:
It feels like this could be done more directly without creating a new set.
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+abstract class AbstractDependencyFilter implements DependencyFilter
+{
+ public boolean isDependencyIncluded( Dependency dependency )
+ {
+ Set<Dependency> set = new LinkedHashSet<>();
+ set.add( dependency );
+
+ set = filter( set );
+ return set.contains( dependency );
+ }
+
+ protected Set<String> splitValues( String csvScopeList )
+ {
+ final String[] scopes = csvScopeList.split( "," );
+ Set<String> excludeScope = new HashSet<>();
+
+ for ( String scope : scopes )
+ {
+ final String cleanScope = scope.trim().toLowerCase( Locale.ROOT );
Review comment:
Interesting. What does Locale.ROOT imply for the lowercasing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] elharo commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643403432
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/ArtifactIdFilter.java
##########
@@ -0,0 +1,98 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Filters dependencies by artifact ID.
+ */
+public class ArtifactIdFilter extends AbstractDependencyFilter
+{
+ private final String includeArtifactId;
+
+ private final String excludeArtifactId;
+
+ public ArtifactIdFilter( String includeArtifactId, String excludeArtifactId )
Review comment:
are these plural? since they have to be split? That is are these really includeArtifactIds and excludeArtifactIds? If so, it might make sense to pass a collection of each instead.
What happens if the same artifact ID is in both lists?
Group IDs not considered here?
In either case it could use a doc comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r667494130
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+
+abstract class AbstractDependencyFilter implements DependencyFilter
Review comment:
I was thinking of closing this PR. We would need the same logic for other goals as well, and this goal could be replaced with the combination of two other goals.
I think you could just create a PR to my branch of you're interested to keep it up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643133929
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+abstract class AbstractDependencyFilter implements DependencyFilter
+{
+ public boolean isDependencyIncluded( Dependency dependency )
+ {
+ Set<Dependency> set = new LinkedHashSet<>();
+ set.add( dependency );
+
+ set = filter( set );
+ return set.contains( dependency );
+ }
+
+ protected Set<String> splitValues( String csvScopeList )
+ {
+ final String[] scopes = csvScopeList.split( "," );
+ Set<String> excludeScope = new HashSet<>();
+
+ for ( String scope : scopes )
+ {
+ final String cleanScope = scope.trim().toLowerCase( Locale.ROOT );
Review comment:
It should have been `Locale.ENGLISH`. I don't want to use no locale (ie system default), because in that case the `toLowerCase()` might behave differently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643182329
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,59 @@
+package org.apache.maven.plugins.dependency.filters;
Review comment:
Yup, because code is similar to artifact-filter's ArtifactFilter which is also in its own package. Also, more filters could be introduced later. Is there a downside of creating a package I do not know?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] antoniolucasnobar commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
antoniolucasnobar commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r668032985
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+
+abstract class AbstractDependencyFilter implements DependencyFilter
Review comment:
Created the PR to your branch: https://github.com/apache/maven-dependency-plugin/pull/145
Should I change anything else? There is a apparently unfinished thread about package names in June 1st.
Do I have to sign any documents? Please let me know.
PS.: I am sorry, butI did not understand why you are thinking in close this PR. Which goals are you talking about? Should they be a part in this PR? I think this PR solves the go-offline problem, which is the subject of MDEP-568.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#issuecomment-898394271
@elharo can you please look over this PR again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] bmarwell commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r643186049
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/FilterDependencies.java
##########
@@ -0,0 +1,91 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.unmodifiableList;
+
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.maven.model.Dependency;
+
+/**
+ * Applies null to any number of filters to a given collection of dependencies.
+ */
+public class FilterDependencies
+{
+
+ private final List<DependencyFilter> filters;
+
+ /**
+ * Created new instance.
+ */
+ public FilterDependencies( DependencyFilter... filters )
+ {
+ this.filters = unmodifiableList( asList( filters ) );
+ }
+
+ /**
+ * <p>filter.</p>
+ *
+ * @param dependencies The {@link Dependency}s to filter.
+ * @return The resulting artifacts set.
+ */
+ public Set<Dependency> filter( Collection<Dependency> dependencies )
+ {
+ Set<Dependency> filtered = new HashSet<>( dependencies );
+
+ for ( DependencyFilter filter : filters )
+ {
+ try
+ {
+ filtered = filter.filter( filtered );
+ }
+ catch ( NullPointerException e )
+ {
+ // TODO: log
Review comment:
copied from FilterArtifacts, will just remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-plugin] antoniolucasnobar commented on a change in pull request #135: [MDEP-568] go offline filter
Posted by GitBox <gi...@apache.org>.
antoniolucasnobar commented on a change in pull request #135:
URL: https://github.com/apache/maven-dependency-plugin/pull/135#discussion_r666972669
##########
File path: src/main/java/org/apache/maven/plugins/dependency/filters/AbstractDependencyFilter.java
##########
@@ -0,0 +1,50 @@
+package org.apache.maven.plugins.dependency.filters;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+
+abstract class AbstractDependencyFilter implements DependencyFilter
Review comment:
Hello @bmarwell ,
Since you created this abstract class, maybe it could contain more than just `splitValues` method. The inherited classes are very similar IMO; This way, this abstract class could have almost all logic here, with the filter, include and exclude methods; It is just a suggestion to avoid code duplication.
Below an example of the abstract class and one of its child. Even the `filterexcludeIds` and `filterincludeIds` could be simplified because they share similar logic.
As ScopeFilter has specific treatment to split include and exclue ids, I created two functions `splitIncludeIds` and `splitExcludeIds`
```java
abstract class AbstractDependencyFilter implements DependencyFilter
{
protected final String includeIds;
protected final String excludeIds;
public AbstractDependencyFilter(String includeIds, String excludeIds)
{
this.includeIds = includeIds == null ? "" : includeIds;
this.excludeIds = excludeIds == null ? "" : excludeIds;
}
@Override
public Set<Dependency> filter(Set<Dependency> dependencies)
{
Set<Dependency> filtered = new HashSet<>(dependencies);
filtered = filterincludeIds(filtered);
filtered = filterexcludeIds(filtered);
return filtered;
}
private Set<Dependency> filterexcludeIds(Set<Dependency> dependencies)
{
if (excludeIds.trim().isEmpty()) {
return dependencies;
}
final Set<String> excludedIds = splitExcludeIds(excludeIds);
Set<Dependency> filtered = new HashSet<>(dependencies.size());
for (Dependency dependency : dependencies) {
if (excludedIds.contains(getContainsProperty(dependency))) {
continue;
}
filtered.add(dependency);
}
return filtered;
}
private Set<Dependency> filterincludeIds(Set<Dependency> dependencies)
{
if (includeIds.trim().isEmpty()) {
return dependencies;
}
Set<String> includedIds = splitIncludeIds(includeIds);
Set<Dependency> filtered = new HashSet<>(dependencies.size());
for (Dependency dependency : dependencies) {
if (includedIds.contains(getContainsProperty(dependency))) {
filtered.add(dependency);
}
}
return filtered;
}
protected Set<String> splitExcludeIds(String includeIds) {
return splitValues(includeIds);
}
protected Set<String> splitIncludeIds(String includeIds) {
return splitValues(includeIds);
}
protected Set<String> splitValues(String excludeIds) {
// ommited here
}
abstract protected String getContainsProperty(Dependency dependency);
}
```
```java
public class GroupIdFilter extends AbstractDependencyFilter
{
public GroupIdFilter(String includeGroupIds, String excludeGroupIds)
{
super(includeGroupIds, excludeGroupIds);
}
protected String getContainsProperty(Dependency dependency) {
return dependency.getGroupId();
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org