You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "bmarwell (via GitHub)" <gi...@apache.org> on 2024/03/02 22:43:33 UTC
[PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
bmarwell opened a new pull request, #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25
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/MSCRIPTING) 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 `[MSCRIPTING-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MSCRIPTING-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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510256280
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
ScriptEngineManager manager = new ScriptEngineManager();
ScriptEngine engine = getEngine(manager);
+ if (ContextAwareEngine.class.isInstance(engine)) {
Review Comment:
```suggestion
if (engine instanceof ContextAwareEngine) {
```
That would be sufficient to me. Yes, it should have already known its log, but THAT would be a public API change.
--
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
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510257385
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
Review Comment:
OK. In that case can this method be non-public? Or is there a missing `@Override` annotation to add 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
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell merged PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25
--
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
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510257161
##########
src/test/java/org/apache/maven/plugins/scripting/engine/JavaEngineTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.plugins.scripting.engine;
+
+/*
+ * 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 javax.script.ScriptEngineManager;
+import javax.script.ScriptException;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+public class JavaEngineTest {
+ @Test
+ public void run() throws ScriptException {
+ assertNull(System.getProperty("JavaEngineTest.run"));
+ new ScriptEngineManager()
+ .getEngineByExtension("java")
+ .eval("System.setProperty(\"JavaEngineTest.run\",\"yes\");");
+ assertEquals("yes", System.getProperty("JavaEngineTest.run"));
+ System.clearProperty("JavaEngineTest.run");
Review Comment:
If the test fails, the JVM will terminate very soon… can do, but it won't have any runtime impact.
--
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
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510262894
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
Review Comment:
I changed it to protected. So are the other methods already. WDYT?
--
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
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510287391
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
Review Comment:
Guess we don't care much, there is no real public API, all are internals of the mojo so private or public are likely equivalent to be extreme in the way to say 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510252301
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
ScriptEngineManager manager = new ScriptEngineManager();
ScriptEngine engine = getEngine(manager);
+ if (ContextAwareEngine.class.isInstance(engine)) {
Review Comment:
This feels somewhat inside out to me. I really wouldn't expect the log to be injected here. It feels like the engine should already know its log.
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
Review Comment:
This changes existing public API. Consider adding a new method instead, especially since this argument isn't used on all paths.
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
Review Comment:
nit: no period per Oracle guidelines
##########
src/test/java/org/apache/maven/plugins/scripting/engine/JavaEngineTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.plugins.scripting.engine;
+
+/*
+ * 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 javax.script.ScriptEngineManager;
+import javax.script.ScriptException;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+public class JavaEngineTest {
+ @Test
+ public void run() throws ScriptException {
+ assertNull(System.getProperty("JavaEngineTest.run"));
+ new ScriptEngineManager()
+ .getEngineByExtension("java")
+ .eval("System.setProperty(\"JavaEngineTest.run\",\"yes\");");
+ assertEquals("yes", System.getProperty("JavaEngineTest.run"));
+ System.clearProperty("JavaEngineTest.run");
Review Comment:
probably belong in a finally block in case the test fails
##########
src/main/java/org/apache/maven/plugins/scripting/engine/JavaEngineFactory.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.plugins.scripting.engine;
+
+/*
+ * 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 javax.script.ScriptEngine;
+import javax.script.ScriptEngineFactory;
+
+import java.util.List;
+import java.util.stream.Stream;
+
+import static java.util.Collections.singletonList;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A java engine factory to be able to script in plain java in the build.
+ */
+public class JavaEngineFactory implements ScriptEngineFactory {
+ @Override
+ public String getEngineName() {
+ return "Maven-Scripting-Java-Engine";
+ }
+
+ @Override
+ public String getEngineVersion() {
+ return "1.0";
+ }
+
+ @Override
+ public List<String> getExtensions() {
+ return singletonList("java");
+ }
+
+ @Override
+ public List<String> getMimeTypes() {
+ return singletonList("application/java");
+ }
+
+ @Override
+ public List<String> getNames() {
+ return Stream.concat(
+ Stream.concat(getMimeTypes().stream(), getExtensions().stream()),
+ Stream.of(getEngineName(), getLanguageName()))
+ .distinct()
+ .collect(toList());
+ }
+
+ @Override
+ public String getLanguageName() {
+ return "java";
+ }
+
+ @Override
+ public String getLanguageVersion() {
+ return System.getProperty("java.version", "8");
+ }
+
+ @Override
+ public Object getParameter(String key) {
+ if (key.equals("javax.script.engine_version")) {
+ return getEngineVersion();
+ }
+ if (key.equals("javax.script.engine")) {
+ return getEngineName();
+ }
+ if (key.equals("javax.script.language")) {
+ return getLanguageName();
+ }
+ if (key.equals("javax.script.language_version")) {
+ return getLanguageVersion();
+ }
+ return null;
+ }
+
+ @Override
+ public String getMethodCallSyntax(String obj, String m, String... args) {
Review Comment:
m --> method
--
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
Re: [PR] [MSCRIPTING-9] Support java scripting [maven-scripting-plugin]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #25:
URL: https://github.com/apache/maven-scripting-plugin/pull/25#discussion_r1510256106
##########
src/main/java/org/apache/maven/plugins/scripting/AbstractScriptEvaluator.java:
##########
@@ -32,13 +35,17 @@ abstract class AbstractScriptEvaluator {
/**
* @param bindings not null bindings to provide to the script to execute
+ * @param log engine logger if context aware.
* @return the possibly null result the script produced
* @throws UnsupportedScriptEngineException when the engine is not configured correctly
* @throws ScriptException if an error occurs in script.
*/
- public final Object eval(Bindings bindings) throws ScriptException, UnsupportedScriptEngineException {
+ public final Object eval(Bindings bindings, Log log) throws ScriptException, UnsupportedScriptEngineException {
Review Comment:
It is not really public as the class is not public. I have an idea how to get rid of this (https://github.com/apache/maven-scripting-plugin/pull/26) but since the Log could be passed in without loosing its type, I would prefer this (non-public!) change.
--
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