You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2023/09/14 08:24:12 UTC
[GitHub] [maven] gnodet opened a new pull request, #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
gnodet opened a new pull request, #1244:
URL: https://github.com/apache/maven/pull/1244
On top of #1243, supersedes #1208
- [MNG-7820] Remove dependency on plexus-utils StringUtils / FileUtils
- [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
--
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] gnodet commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1330176084
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Just so I understand, why do you insist on splitting things that are clearly related to a single issue ? Is that just to slow down things and give me more work :-) ? I do agree on splitting when changes are unrelated, but here the one and only goal is to get rid of plexus-utils. Fwiw, if I can't go there for whatever reason, I'll revert all the changes as they don't make any sense to me as individual changes.
--
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] michael-o commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1327501134
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Would it make sense to skip this class?
--
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] gnodet closed pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
URL: https://github.com/apache/maven/pull/1244
--
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] gnodet commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1330176084
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Just so I understand, why do you insist on splitting things that are clearly related to a single issue ? Is that just to slow down things ? I do agree on splitting when changes are unrelated, but here the one and only goal is to get rid of plexus-utils. Fwiw, if I can't go there for whatever reason, I'll revert all the changes as they don't make any sense to me as individual changes.
--
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] gnodet commented on pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1244:
URL: https://github.com/apache/maven/pull/1244#issuecomment-1719636744
> > still appears to violate Apache rules on inclusion of third party code
>
> I disagree for the following reasons:
>
> * `MethodMap`, `ClassMap` and `ReflectionValueExtractor` [come from the ASF](https://github.com/codehaus-plexus/plexus-utils/commit/1325c3d2de75da489539cd5f9c6e89bdcfb92c25), [originally in Maven Core](https://svn.apache.org/viewvc/maven/components/trunk/maven-core/src/main/java/org/apache/maven/util/introspection/?pathrev=163742) and have not undergone major modifications since
> * the `Xpp3PerfTest` has been written by me, so I hereby donate it to the ASF
> * the `Os` and `DAG` (now `Graph`) have been rewritten
@elharo I've raised https://issues.apache.org/jira/browse/LEGAL-655
--
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] elharo commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1328551165
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Os, DAG and ReflectionValueExtractor should be at least three PRs, not one.
--
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] elharo commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1325767509
##########
maven-core/src/main/java/org/apache/maven/plugin/MethodMap.java:
##########
@@ -0,0 +1,395 @@
+/*
+ * 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.
+ */
+package org.apache.maven.plugin;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Hashtable;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+class MethodMap {
+ private static final int MORE_SPECIFIC = 0;
+
+ private static final int LESS_SPECIFIC = 1;
+
+ private static final int INCOMPARABLE = 2;
+
+ /**
+ * Keep track of all methods with the same name.
+ */
+ Map<String, List<Method>> methodByNameMap = new Hashtable<>();
+
+ /**
+ * Add a method to a list of methods by name. For a particular class we are keeping track of all the methods with
+ * the same name.
+ *
+ * @param method The method
+ */
+ public void add(Method method) {
+ String methodName = method.getName();
+
+ List<Method> l = get(methodName);
Review Comment:
l --> list
In general, avoid abbreviations and especially avoid the single letter L which is confusable with the digit one in many fonts
--
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] elharo commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1328539508
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Yes, we do, but one thing at a time.
--
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] gnodet commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1328543020
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
@michael-o @elharo so I don't really understand what you both want / imply. Could you be more specific please?
--
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] gnodet commented on pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1244:
URL: https://github.com/apache/maven/pull/1244#issuecomment-1719328165
> still appears to violate Apache rules on inclusion of third party code
I disagree for the following reasons:
* `MethodMap`, `ClassMap` and `ReflectionValueExtractor` [come from the ASF](https://github.com/codehaus-plexus/plexus-utils/commit/1325c3d2de75da489539cd5f9c6e89bdcfb92c25), [originally in Maven Core](https://svn.apache.org/viewvc/maven/components/trunk/maven-core/src/main/java/org/apache/maven/util/introspection/?pathrev=163742) and have not undergone major modifications since
* the `Xpp3PerfTest` has been written by me, so I hereby donate it to the ASF
* the `Os` and `DAG` (now `Graph`) have been rewritten
--
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] gnodet commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1328253129
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Well, the point is to get rid of plexus-utils, so we need a replacement for that one too.
--
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] elharo commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1328550554
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Split out all changes involving the Os class into a separate PR. Do not change the Os class or its usages in this PR, so that easier changes aren't gated on more complex work.
--
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] elharo commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1330308314
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
No, I want separate PRs. Issues can be bigger than a single PR. See https://google.github.io/eng-practices/review/developer/small-cls.html for more discussion.
--
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] gnodet commented on a diff in pull request #1244: [MNG-7820] Get rid of plexus-utils Os, DAG and ReflectionValueExtractor
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1244:
URL: https://github.com/apache/maven/pull/1244#discussion_r1330186075
##########
maven-model-builder/src/main/java/org/apache/maven/utils/Os.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * 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.
+ */
+package org.apache.maven.utils;
+
+import java.util.Locale;
+import java.util.stream.Stream;
+
+/**
+ * OS support
+ */
+public class Os {
Review Comment:
Or maybe what you wanted was different commits on a single PR so that's it can be reviewed per commit ? That would have made more sense to me...
--
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