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 2022/10/09 18:36:24 UTC
[GitHub] [maven] gnodet opened a new pull request, #819: Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator
gnodet opened a new pull request, #819:
URL: https://github.com/apache/maven/pull/819
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) 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.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] SUMMARY`, where you replace `MNG-XXX`
and `SUMMARY` 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.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
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.
- [ ] 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)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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 #819: [MNG-7553] Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1274152174
I'm going to split that PR in 2. This one will be kept for the modifications on the v4 api : the new services and the related discussions.
I'll create a separate PR for the restoration of the API.
--
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 pull request #819: Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1272603577
Can you elaborate on the change? When was it broken?
--
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 #819: Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1272627167
> Can you elaborate on the change? When was it broken?
When the v4 api was merged. A few services were migrated to the new API, but that was a mistake because they are in use by plugins. This is the case for the `ToolchainsBuilder` and the `MojoDescriptorCreator` as reported on https://lists.apache.org/list.html?dev@maven.apache.org. Whether those are considered internal or part of the real api could be discussed, but they were broken, as now is not the time to break compatibility.
This PR then restores those services and add 2 new services to the new API to build settings and toolchains.
I need to investigate why the IT is failing.
--
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 #819: [MNG-7553] Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator
Posted by GitBox <gi...@apache.org>.
elharo commented on code in PR #819:
URL: https://github.com/apache/maven/pull/819#discussion_r991663543
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Session.java:
##########
@@ -56,9 +56,21 @@
@Nonnull
SessionData getData();
+ /**
+ * Gets the user properties to use for interpolation. The user properties have been configured directly by the user
+ * on his discretion, e.g. via the {@code -Dkey=value} parameter on the command line.
Review Comment:
delete "on his discretion"
, --> ;
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilder.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.
Review Comment:
what is "effective settings"? How does that differ from other settings?
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Session.java:
##########
@@ -56,9 +56,21 @@
@Nonnull
SessionData getData();
+ /**
+ * Gets the user properties to use for interpolation. The user properties have been configured directly by the user
+ * on his discretion, e.g. via the {@code -Dkey=value} parameter on the command line.
+ *
+ * @return The user properties, never {@code null}.
+ */
@Nonnull
Map<String, String> getUserProperties();
+ /**
+ * Gets the system properties to use for interpolation. The system properties are collected from the runtime
+ * environment like {@link System#getProperties()} and environment variables.
Review Comment:
like --> such as
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilder.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.
+ */
+public interface SettingsBuilder extends Service
+{
+
+ /**
+ * Builds the effective settings of the specified settings files.
+ *
+ * @param request The settings building request that holds the parameters, must not be {@code null}.
Review Comment:
The --> the (per Oracle, API doc params do not begin with a capital letter
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/BuilderProblemSeverity.java:
##########
@@ -27,7 +27,7 @@
* @since 4.0
*/
@Experimental
-public enum ProjectBuilderProblemSeverity
+public enum BuilderProblemSeverity
Review Comment:
rename seems unnecessary
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ProjectBuilder.java:
##########
@@ -51,13 +51,13 @@ public interface ProjectBuilder extends Service
* Creates a {@link org.apache.maven.api.Project} from a POM file.
*
* @param session The {@link Session}, must not be {@code null}.
- * @param source The {@link ProjectBuilderSource}, must not be {@code null}.
- * @throws ProjectBuilderException if the project cannot be created
+ * @param source The {@link Source}, must not be {@code null}.
+ * @throws ProjectBuilderException if the project can not be created
Review Comment:
revert, cannot is one word
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ProjectBuilder.java:
##########
@@ -51,13 +51,13 @@ public interface ProjectBuilder extends Service
* Creates a {@link org.apache.maven.api.Project} from a POM file.
*
* @param session The {@link Session}, must not be {@code null}.
- * @param source The {@link ProjectBuilderSource}, must not be {@code null}.
- * @throws ProjectBuilderException if the project cannot be created
+ * @param source The {@link Source}, must not be {@code null}.
Review Comment:
The --> the
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Session.java:
##########
@@ -56,9 +56,21 @@
@Nonnull
SessionData getData();
+ /**
Review Comment:
Javadoc additions to existing API are useful but can be included in a separate PR that is easier to approve.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderException.java:
##########
@@ -0,0 +1,43 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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 org.apache.maven.api.annotations.Experimental;
+
+/**
+ * The Exception class throw by the {@link SettingsBuilder}.
+ *
+ * @since 4.0
+ */
+@Experimental
+public class SettingsBuilderException
+ extends MavenException
+{
+ /**
+ * @param message The message to give.
+ * @param e The {@link Exception}.
Review Comment:
don't repeat type information in doc comments. It's redundant. Instead explain what this exception is
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilder.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.
+ */
+public interface SettingsBuilder extends Service
+{
+
+ /**
+ * Builds the effective settings of the specified settings files.
+ *
+ * @param request The settings building request that holds the parameters, must not be {@code null}.
+ * @return The result of the settings building, never {@code null}.
+ * @throws SettingsBuilderException If the effective settings could not be built.
Review Comment:
if the effective settings could not be built
--
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 #819: [MNG-7553] Restore compatibility on SettingsBuilder, ToolchainsBuilder and MojoDescriptorCreator
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #819:
URL: https://github.com/apache/maven/pull/819#discussion_r991605833
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilder.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.
+ */
+public interface SettingsBuilder extends Service
+{
+
+ /**
+ * Builds the effective settings of the specified settings files.
+ *
+ * @param request The settings building request that holds the parameters, must not be {@code null}.
+ * @return The result of the settings building, never {@code null}.
+ * @throws SettingsBuilderException If the effective settings could not be built.
+ */
+ @Nonnull
+ SettingsBuilderResult build( @Nonnull SettingsBuilderRequest request );
Review Comment:
Same question here.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ProjectBuilderResult.java:
##########
@@ -70,7 +70,7 @@
* @return The problems that were encountered during the project building, can be empty but never {@code null}.
*/
@Nonnull
- Collection<ProjectBuilderProblem> getProblems();
+ Collection<BuilderProblem> getProblems();
Review Comment:
Stupid question: Is it really a problem with the builder or a problem with the build?
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderRequest.java:
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+/**
+ * Collects settings that control the building of effective settings.
+ */
+@Experimental
+@Immutable
+public interface SettingsBuilderRequest
+{
+
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global settings path.
+ *
+ * @return The global settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalSettingsPath();
+
+ /**
+ * Gets the global settings source.
+ *
+ * @return The global settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalSettingsSource();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderRequest.java:
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+/**
+ * Collects settings that control the building of effective settings.
+ */
+@Experimental
+@Immutable
+public interface SettingsBuilderRequest
+{
+
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global settings path.
+ *
+ * @return The global settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalSettingsPath();
+
+ /**
+ * Gets the global settings source.
+ *
+ * @return The global settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalSettingsSource();
+
+ /**
+ * Gets the user settings path.
+ *
+ * @return The user settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserSettingsPath();
+
+ /**
+ * Gets the user settings source.
+ *
+ * @return The user settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getUserSettingsSource();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderRequest.java:
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+/**
+ * Collects settings that control the building of effective settings.
+ */
+@Experimental
+@Immutable
+public interface SettingsBuilderRequest
+{
+
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global settings path.
+ *
+ * @return The global settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalSettingsPath();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderRequest.java:
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+/**
+ * Collects settings that control the building of effective settings.
+ */
+@Experimental
+@Immutable
+public interface SettingsBuilderRequest
+{
+
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global settings path.
+ *
+ * @return The global settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalSettingsPath();
+
+ /**
+ * Gets the global settings source.
+ *
+ * @return The global settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalSettingsSource();
+
+ /**
+ * Gets the user settings path.
+ *
+ * @return The user settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserSettingsPath();
+
+ /**
+ * Gets the user settings source.
+ *
+ * @return The user settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getUserSettingsSource();
+
+ @Nonnull
+ static SettingsBuilderRequest build( @Nonnull Session session,
+ @Nonnull Source globalSettingsSource,
+ @Nonnull Source userSettingsSource )
+ {
+ return builder()
+ .session( nonNull( session, "session can not be null" ) )
+ .globalSettingsSource( nonNull( globalSettingsSource, "globalSettingsSource can not be null" ) )
+ .userSettingsSource( nonNull( userSettingsSource, "userSettingsSource can not be null" ) )
+ .build();
Review Comment:
cannot
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderRequest.java:
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+/**
+ * Collects settings that control the building of effective settings.
+ */
+@Experimental
+@Immutable
+public interface SettingsBuilderRequest
+{
+
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global settings path.
+ *
+ * @return The global settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalSettingsPath();
+
+ /**
+ * Gets the global settings source.
+ *
+ * @return The global settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalSettingsSource();
+
+ /**
+ * Gets the user settings path.
+ *
+ * @return The user settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserSettingsPath();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilderRequest.java:
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+/**
+ * Collects settings that control the building of effective settings.
+ */
+@Experimental
+@Immutable
+public interface SettingsBuilderRequest
+{
+
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global settings path.
+ *
+ * @return The global settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalSettingsPath();
+
+ /**
+ * Gets the global settings source.
+ *
+ * @return The global settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalSettingsSource();
+
+ /**
+ * Gets the user settings path.
+ *
+ * @return The user settings path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserSettingsPath();
+
+ /**
+ * Gets the user settings source.
+ *
+ * @return The user settings source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getUserSettingsSource();
+
+ @Nonnull
+ static SettingsBuilderRequest build( @Nonnull Session session,
+ @Nonnull Source globalSettingsSource,
+ @Nonnull Source userSettingsSource )
+ {
+ return builder()
+ .session( nonNull( session, "session can not be null" ) )
+ .globalSettingsSource( nonNull( globalSettingsSource, "globalSettingsSource can not be null" ) )
+ .userSettingsSource( nonNull( userSettingsSource, "userSettingsSource can not be null" ) )
+ .build();
+ }
+
+ @Nonnull
+ static SettingsBuilderRequest build( @Nonnull Session session,
+ @Nonnull Path globalSettingsPath,
+ @Nonnull Path userSettingsPath )
+ {
+ return builder()
+ .session( nonNull( session, "session can not be null" ) )
+ .globalSettingsPath( nonNull( globalSettingsPath, "globalSettingsPath can not be null" ) )
+ .userSettingsPath( nonNull( userSettingsPath, "userSettingsPath can not be null" ) )
+ .build();
+ }
Review Comment:
cannot
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderRequest.java:
##########
@@ -0,0 +1,196 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+public interface ToolchainsBuilderRequest
+{
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global Toolchains path.
+ *
+ * @return The global Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalToolchainsPath();
+
+ /**
+ * Gets the global Toolchains source.
+ *
+ * @return The global Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalToolchainsSource();
+
+ /**
+ * Gets the user Toolchains path.
+ *
+ * @return The user Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserToolchainsPath();
+
+ /**
+ * Gets the user Toolchains source.
+ *
+ * @return The user Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getUserToolchainsSource();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderRequest.java:
##########
@@ -0,0 +1,196 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+public interface ToolchainsBuilderRequest
+{
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global Toolchains path.
+ *
+ * @return The global Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalToolchainsPath();
+
+ /**
+ * Gets the global Toolchains source.
+ *
+ * @return The global Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalToolchainsSource();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderRequest.java:
##########
@@ -0,0 +1,196 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+public interface ToolchainsBuilderRequest
+{
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global Toolchains path.
+ *
+ * @return The global Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalToolchainsPath();
+
+ /**
+ * Gets the global Toolchains source.
+ *
+ * @return The global Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalToolchainsSource();
+
+ /**
+ * Gets the user Toolchains path.
+ *
+ * @return The user Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserToolchainsPath();
Review Comment:
Javadoc does not match code
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderRequest.java:
##########
@@ -0,0 +1,196 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+public interface ToolchainsBuilderRequest
+{
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global Toolchains path.
+ *
+ * @return The global Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalToolchainsPath();
+
+ /**
+ * Gets the global Toolchains source.
+ *
+ * @return The global Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalToolchainsSource();
+
+ /**
+ * Gets the user Toolchains path.
+ *
+ * @return The user Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserToolchainsPath();
+
+ /**
+ * Gets the user Toolchains source.
+ *
+ * @return The user Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getUserToolchainsSource();
+
+ @Nonnull
+ static ToolchainsBuilderRequest build( @Nonnull Session session,
+ @Nonnull Source globalToolchainsSource,
+ @Nonnull Source userToolchainsSource )
+ {
+ return builder()
+ .session( nonNull( session, "session can not be null" ) )
+ .globalToolchainsSource( nonNull( globalToolchainsSource, "globalToolchainsSource can not be null" ) )
+ .userToolchainsSource( nonNull( userToolchainsSource, "userToolchainsSource can not be null" ) )
+ .build();
+ }
Review Comment:
cannot
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderRequest.java:
##########
@@ -0,0 +1,196 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+public interface ToolchainsBuilderRequest
+{
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global Toolchains path.
+ *
+ * @return The global Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalToolchainsPath();
Review Comment:
Javadoc does not match code
##########
maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java:
##########
@@ -228,9 +228,9 @@ public String getMessage()
}
@Override
- public ProjectBuilderProblemSeverity getSeverity()
+ public BuilderProblemSeverity getSeverity()
Review Comment:
Why isn't it `BuilderProblem.Severity`?
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderRequest.java:
##########
@@ -0,0 +1,196 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+import java.util.Optional;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.NotThreadSafe;
+import org.apache.maven.api.annotations.Nullable;
+
+import static org.apache.maven.api.services.BaseRequest.nonNull;
+
+public interface ToolchainsBuilderRequest
+{
+ @Nonnull
+ Session getSession();
+
+ /**
+ * Gets the global Toolchains path.
+ *
+ * @return The global Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getGlobalToolchainsPath();
+
+ /**
+ * Gets the global Toolchains source.
+ *
+ * @return The global Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getGlobalToolchainsSource();
+
+ /**
+ * Gets the user Toolchains path.
+ *
+ * @return The user Toolchains path or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Path> getUserToolchainsPath();
+
+ /**
+ * Gets the user Toolchains source.
+ *
+ * @return The user Toolchains source or {@code null} if none.
+ */
+ @Nonnull
+ Optional<Source> getUserToolchainsSource();
+
+ @Nonnull
+ static ToolchainsBuilderRequest build( @Nonnull Session session,
+ @Nonnull Source globalToolchainsSource,
+ @Nonnull Source userToolchainsSource )
+ {
+ return builder()
+ .session( nonNull( session, "session can not be null" ) )
+ .globalToolchainsSource( nonNull( globalToolchainsSource, "globalToolchainsSource can not be null" ) )
+ .userToolchainsSource( nonNull( userToolchainsSource, "userToolchainsSource can not be null" ) )
+ .build();
+ }
+
+ @Nonnull
+ static ToolchainsBuilderRequest build( @Nonnull Session session,
+ @Nonnull Path globalToolchainsPath,
+ @Nonnull Path userToolchainsPath )
+ {
+ return builder()
+ .session( nonNull( session, "session can not be null" ) )
+ .globalToolchainsPath( nonNull( globalToolchainsPath, "globalToolchainsPath can not be null" ) )
+ .userToolchainsPath( nonNull( userToolchainsPath, "userToolchainsPath can not be null" ) )
+ .build();
+ }
Review Comment:
cannot
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainsBuilderException.java:
##########
@@ -0,0 +1,43 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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 org.apache.maven.api.annotations.Experimental;
+
+/**
+ * The Exception class throw by the {@link ToolchainsBuilder}.
+ *
+ * @since 4.0
+ */
+@Experimental
+public class ToolchainsBuilderException
+ extends MavenException
+{
+ /**
+ * @param message The message to give.
+ * @param e The {@link Exception}.
+ */
+ public ToolchainsBuilderException( String message, Exception e )
Review Comment:
Same here
--
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 #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1274257056
> I see two general issues/questions here:
I would rather have had those discussions when I asked for feedback on the API, but I suppose it's not too late ;-)
See my answers / reasoning below.
>
> * We have already briefly discussed the benefit/overrating of `Optional`. Can you explain why it makes sense here? I mean `X != null` is as good as here.
I slightly disagree. The main differences are that when using an `Optional`, the code carries the semantics and actually forces the caller to deal with it, whereas just if you simply return a value (the optionals are only used as return values), you have to get to the definition, read the javadoc, so that you can know if you need to deal with a possible null value. There are very often places in our code where you see both null checks and not.
This also hurts code concision. A simple example found in the code:
```
if ( getBaseVersionInternal() != null )
{
sb.append( getBaseVersionInternal() );
}
else
{
sb.append( versionRange.toString() );
}
```
This could be rewritten as:
```
sb.append( getBaseVersionInternal().orElse( versionRange.toString() ) );
```
This is actually related to the fact that the maven coding style is very sparse. I actually find that quite difficult to work with (even if I'm getting used to it). That may be due to the fact I'm working almost exclusively on a laptop, so the height of it is limited, and only seing 30-40 lines of code at the same time becomes a problem when nearly 50% of those don't really convey any semantic (being blank lines or braces usually). It means more time to scroll through the code back and forth to understand it. I don't like that. Streams are another way to get around empty lines, just because often, you simply don't need braces when using them, so you have 1 line instead of 3.
So generally speaking, the more semantic carried by the code, the better. I agree to not overuse `Optional`, I just don't think they are at this point.
> * I have a general problem with the terms `XBuilderRequest`/`XBuilderResult`/`XBuilderException`. I think this is incorrectly names since it implies that there is something with the builder and not the build itself. In other spots we use the terms `XBuildingRequest`/`XBuildingResult`/`XBuildingException`/`XBuildingProblem` or just `Build`. @elharo Sample: https://github.com/apache/maven/blob/35b93b0a589752cc88105623a2ddf9e52b56c1ce/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
I'm actually seeing the terms as _a request to the X builder_, _the result returned by X builder_, _the exception thrown by X builder_. We can't use quotes, but think about those as _XBuilder's request_, _XBuilder's result_, _XBuilder's exception_. I actually think those make more sense than what we have in other places. It's also easier to see the relationship between the service and its related request/result/exception.
Also, if you think each service usually throws _its own exception_. If we move the semantic to _what action is performed_ instead of _who does the action_, the exceptions would have to be more shared between services. However (and that's still missing right now), a bunch of exception are used to carry the _context_ in which the exception was thrown. So you end up with untyped `source` fields or similar things.
--
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 #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #819:
URL: https://github.com/apache/maven/pull/819#discussion_r992123767
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilder.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.
Review Comment:
I think "effective" here means an aggregation/merge of the global and user settings.
> what is "effective settings"? How does that differ from other settings?
--
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 #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
elharo commented on code in PR #819:
URL: https://github.com/apache/maven/pull/819#discussion_r992238726
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/SettingsBuilder.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.maven.api.services;
+
+/*
+ * 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.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Builds the effective settings from a user settings file and/or a global settings file.
Review Comment:
You need to put this in the docs, perhaps rewriting without the word effective
--
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 #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #819:
URL: https://github.com/apache/maven/pull/819#discussion_r991989660
##########
maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java:
##########
@@ -228,9 +228,9 @@ public String getMessage()
}
@Override
- public ProjectBuilderProblemSeverity getSeverity()
+ public BuilderProblemSeverity getSeverity()
Review Comment:
No reason, I'll move it as an inner enum.
--
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 pull request #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1274619037
> > I see two general issues/questions here:
> > I would rather have had those discussions when I asked for feedback on the API, but I suppose it's not too late ;-)
> > See my answers / reasoning below.
>
> > * We have already briefly discussed the benefit/overrating of `Optional`. Can you explain why it makes sense here? I mean `X != null` is as good as here.
>
> I slightly disagree. The main differences are that when using an `Optional`, the code carries the semantics and actually forces the caller to deal with it, whereas just if you simply return a value (the optionals are only used as return values), you have to get to the definition, read the javadoc, so that you can know if you need to deal with a possible null value. There are very often places in our code where you see both null checks and not and people end up adding null checks, just in case.
>
> This also hurts code concision. A simple example found in the code:
>
> ```
> if ( getBaseVersionInternal() != null )
> {
> sb.append( getBaseVersionInternal() );
> }
> else
> {
> sb.append( versionRange.toString() );
> }
> ```
>
> This could be rewritten as:
>
> ```
> sb.append( getBaseVersionInternal().orElse( versionRange.toString() ) );
> ```
>
> This is actually related to the fact that the maven coding style is very sparse. I actually find that quite difficult to work with (even if I'm getting used to it). That may be due to the fact I'm working almost exclusively on a laptop, so the height of it is limited, and only seing 30-40 lines of code at the same time becomes a problem when nearly 50% of those don't really convey any semantic (being blank lines or braces usually). It means more time to scroll through the code back and forth to understand it. I don't like that. Streams are another way to get around empty lines, just because often, you simply don't need braces when using them, so you have 1 line instead of 3.
>
> So generally speaking, the more semantic carried by the code, the better. I agree to not overuse `Optional`, I just don't think they are at this point. That said, the use of `Nullable` / `NonNull` annotations does help with semantics already, and I would not be opposed to removing them on requests / results objects.
This reasoning I can follow where `Optional` adds value. I don't like like to proclaim `Optional` to be the new `null` which is non sense. No objections here.
> > * I have a general problem with the terms `XBuilderRequest`/`XBuilderResult`/`XBuilderException`. I think this is incorrectly names since it implies that there is something with the builder and not the build itself. In other spots we use the terms `XBuildingRequest`/`XBuildingResult`/`XBuildingException`/`XBuildingProblem` or just `Build`. @elharo Sample: https://github.com/apache/maven/blob/35b93b0a589752cc88105623a2ddf9e52b56c1ce/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
>
> I'm actually seeing the terms as _a request to the X builder_, _the result returned by X builder_, _the exception thrown by X builder_. We can't use quotes, but think about those as _XBuilder's request_, _XBuilder's result_, _XBuilder's exception_. I actually think those make more sense than what we have in other places. It's also easier to see the relationship between the service and its related request/result/exception. Also, if you think each service usually throws _its own exception_. If we move the semantic to _what action is performed_ instead of _who does the action_, the exceptions would have to be more shared between services. However (and that's still missing on some right now), a bunch of exception are used to carry the _context_ in which the exception was thrown.
>
> Another point that just came to my mind is that wording like `DependencyCollectionRequest` is not intuitive either: the term `Collection` in java is immediately associated to a... `java.util.Collection`, and not to the act of collecting...
Let me crunch on this. @elharo What is your opinion on the style here?
--
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 #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1291117534
This PR has been split with #852, #853, #854 for ease of review.
--
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 #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
Posted by GitBox <gi...@apache.org>.
gnodet closed pull request #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services
URL: https://github.com/apache/maven/pull/819
--
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